ADE CLI / PR workflow fixes and integration lane ownership#185
Conversation
|
@copilot review but do not make fixes |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIntroduces centralized ADE CLI guidance constants and substantially expands the dynamic ADE action framework with role-based access control, new domains (layout, tiling_tree, graph_state), and enhanced action metadata. Multiple components adopt this shared guidance instead of hardcoding prompts. PR service gains draft description support with optional base branch context. Runtime bootstrapping extends to wire 20+ new optional services. CLI system is refactored with improved help documentation, project root discovery, and PR linking. Comprehensive test coverage added across action execution, CLI validation, and agent prompt expectations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| const projectRoot = options.projectRoot | ||
| ?? (process.env.ADE_PROJECT_ROOT?.trim() ? path.resolve(process.env.ADE_PROJECT_ROOT.trim()) : findProjectRoot(process.cwd())); | ||
| ?? (process.env.ADE_PROJECT_ROOT?.trim() ? path.resolve(process.env.ADE_PROJECT_ROOT.trim()) : discovered.projectRoot); | ||
| const workspaceRoot = options.workspaceRoot |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
This change stops workspaceRoot from following an explicit --project-root / ADE_PROJECT_ROOT override:
// apps/ade-cli/src/cli.ts
const projectRoot = options.projectRoot
?? (process.env.ADE_PROJECT_ROOT?.trim() ? path.resolve(process.env.ADE_PROJECT_ROOT.trim()) : discovered.projectRoot);
const workspaceRoot = options.workspaceRoot
?? (process.env.ADE_WORKSPACE_ROOT?.trim() ? path.resolve(process.env.ADE_WORKSPACE_ROOT.trim()) : discovered.workspaceRoot);I verified that createConnection() passes both values into createAdeRuntime(), and adeRpcServer later uses runtime.workspaceRoot as the default filesystem/process workspace when no lane-specific worktree is selected. That means running ade --project-root /repoA ... from inside /repoB/.ade/worktrees/... will persist state and socket/layout lookups against repo A while file/process/agent operations default to repo B. Default workspaceRoot back to the resolved projectRoot whenever the project root was explicitly overridden, and only use discovered.workspaceRoot for pure auto-discovery.
| ` | ||
| update pull_requests | ||
| set repo_owner = ?, | ||
| set lane_id = ?, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When createFromLane()/linkToLane() reuses an existing PR row by repo_owner/repo_name/github_pr_number, this new update also rewrites that row's lane_id, but nothing updates the matching pr_group_members.lane_id rows. ts // apps/desktop/src/main/services/prs/prService.ts update pull_requests set lane_id = ?, repo_owner = ?, repo_name = ?, github_pr_number = ?, That leaves the PR record attached to the new lane while queue/integration membership, archive guards, and lane deletion cleanup still key off the old lane id (resolveQueueRebaseOverride() queries m.lane_id, and laneService.archive/delete also read/delete pr_group_members by lane_id). The result is inconsistent workflow state: the adopted lane loses its queue/integration context, the old lane can stay falsely blocked from archive/delete, and deleting the new lane can leave stale group-member rows behind. Fix by updating pr_group_members.lane_id for the reused PR in the same transaction, or by avoiding lane_id reassignment when reusing an existing PR row.
| memoryService, | ||
| ctoStateService, | ||
| workerAgentService, | ||
| workerBudgetService, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The expanded AdeRuntime contract now includes workerHeartbeatService, workerTaskSessionService, and linearCredentialService, and createHeadlessLinearServices() already constructs all three, but this return object still stops at workerBudgetService before jumping straight to prService:
// apps/ade-cli/src/bootstrap.ts
workerAgentService,
workerBudgetService,
prService: headlessLinearServices.prService,
fileService: headlessLinearServices.fileService,
flowPolicyService: headlessLinearServices.flowPolicyService,Because @apps/ade-cli/src/cli.ts uses createAdeRuntime() for headless ade actions calls, and @apps/ade-cli/src/adeRpcServer.ts only lists/calls domains when the corresponding runtime service is present, the worker_heartbeat, worker_task_session, and linear_credentials domains remain unavailable in CLI mode even though this PR is wiring them into the desktop socket runtime. Forward those already-created services from headlessLinearServices into the returned AdeRuntime so headless and embedded RPC stay in sync.
arul28
force-pushed
the
ade/cli-prs-fixes-747d7096
branch
from
64f7147 to
51d2f42
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }, | ||
| set(args?: Record<string, unknown>): { layoutId: string; layout: DockLayout } { | ||
| const layoutId = assertNonEmptyString(args?.layoutId, "layoutId"); | ||
| const layout = clampDockLayout(safeObject(args?.layout)); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new synthetic layout/tiling_tree/graph_state ADE action services do not enforce the same required payload contract as the existing IPC APIs. In this file, layout.set turns a missing layout into {}, while tiling_tree.set and graph_state.set turn missing tree/state into null, then persist those values immediately. That means a malformed CLI call like run_ade_action layout.set --input-json '{"layoutId":"main"}' now erases the saved UI state instead of failing fast. The existing desktop contract requires these fields (preload exposes set(layoutId, layout), set(layoutId, tree), set(projectId, state)), so the CLI wrapper should reject absent payloads rather than defaulting to destructive writes. ts // apps/ade-cli/src/adeRpcServer.ts const layout = clampDockLayout(safeObject(args?.layout)); runtime.db.setJson(`dock_layout:${layoutId}`, layout); const tree = Object.prototype.hasOwnProperty.call(args ?? {}, "tree") ? args?.tree : null; runtime.db.setJson(`tiling_tree:${layoutId}`, tree); Validate layout, tree, and state explicitly before persisting so bad input returns invalidParams instead of wiping saved state.
| let title = asOptionalTrimmedString(toolArgs.title); | ||
| let body = typeof toolArgs.body === "string" ? toolArgs.body : null; | ||
| if (!title || body == null) { | ||
| const draft = await prSvc.draftDescription({ laneId }); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When create_pr_from_lane is called with a non-default baseBranch and the caller omits title or body, this handler drafts the PR text without passing that override into the drafting path:
// apps/ade-cli/src/adeRpcServer.ts
if (!title || body == null) {
const draft = await prSvc.draftDescription({ laneId });
title = title || asOptionalTrimmedString(draft.title) || `PR for ${laneId}`;
body = body ?? asOptionalTrimmedString(draft.body) ?? "";
}prService.draftDescription only accepts { laneId } and computes its summary from lane.baseRef...HEAD, while createFromLane later honors the explicit baseBranch override. The result is a PR whose generated title/body describe a different diff than the actual target branch, which is user-visible and can mislead review/merge decisions for non-default-base workflows. Fix by threading baseBranch through DraftPrDescriptionArgs/draftDescription, or by skipping auto-drafting when an override base is supplied.
| work: "chat", | ||
| agents: "agent", | ||
| test: "tests", | ||
| computer: "proof", |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new alias table advertises computer, artifact, and setting, but buildCliPlan only uses that table for help lookup; the execution dispatch below still routes only proof | computer-use | artifacts and settings | config. That means commands like ade computer status, ade artifact list, and ade setting get now fail even though the diff exposes them as supported aliases. ```ts
// apps/ade-cli/src/cli.ts
test: "tests",
computer: "proof",
"computer-use": "proof",
artifact: "proof",
artifacts: "proof",
setting: "settings",
| ?? (process.env.ADE_PROJECT_ROOT?.trim() ? path.resolve(process.env.ADE_PROJECT_ROOT.trim()) : discovered.projectRoot); | ||
| const workspaceRoot = options.workspaceRoot | ||
| ?? (process.env.ADE_WORKSPACE_ROOT?.trim() ? path.resolve(process.env.ADE_WORKSPACE_ROOT.trim()) : projectRoot); | ||
| ?? (process.env.ADE_WORKSPACE_ROOT?.trim() ? path.resolve(process.env.ADE_WORKSPACE_ROOT.trim()) : discovered.workspaceRoot); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When the CLI is invoked from inside .ade/worktrees/<lane> and the caller overrides only --project-root, resolveRoots() now mixes two different defaults: it takes the requested project root, but still keeps the discovered worktree from the current cwd as workspaceRoot.
// apps/ade-cli/src/cli.ts
const discovered = findProjectRoots(process.cwd());
const projectRoot = options.projectRoot
?? (process.env.ADE_PROJECT_ROOT?.trim() ? path.resolve(process.env.ADE_PROJECT_ROOT.trim()) : discovered.projectRoot);
const workspaceRoot = options.workspaceRoot
?? (process.env.ADE_WORKSPACE_ROOT?.trim() ? path.resolve(process.env.ADE_WORKSPACE_ROOT.trim()) : discovered.workspaceRoot);createConnection() passes both values into createAdeRuntime(), and the RPC layer falls back to runtime.workspaceRoot for lane-less operations, so commands can read/write the current lane worktree while talking to a different project's ADE database/socket. Default workspaceRoot to the resolved projectRoot whenever --project-root/ADE_PROJECT_ROOT is supplied and there is no explicit workspace override.
| "savePolicy", | ||
| "validatePolicy", | ||
| ], | ||
| linear_credentials: [ |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
These new allowlist entries widen run_ade_action from project-scoped automation into account/app administration. run_ade_action is still part of the base tool set returned to agent sessions, and the updated tests explicitly initialize an agent caller and assert that github and update are visible, so a worker can now invoke methods like linear_credentials.setToken, github.clearToken, or update.quitAndInstall. That lets an LLM session revoke stored credentials or trigger an app restart/install, which is outside lane/PR workflow scope and can disrupt the operator’s environment. Restrict these domains/actions to trusted roles (external/cto) or add a per-domain authorization check before dispatch.
// apps/ade-cli/src/adeRpcServer.ts
linear_credentials: [
"clearOAuthClientCredentials",
"clearToken",
"getStatus",
"setOAuthClientCredentials",
"setOAuthToken",| const now = nowIso(); | ||
| const existing = getRowForLane(summary.laneId); | ||
| const existing = getRowForLane(summary.laneId) | ||
| ?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
upsertRow now reuses an existing PR row by repo/PR number and reassigns it to the new lane, but it never clears or rewrites that row's pr_group_members entries.
// apps/desktop/src/main/services/prs/prService.ts
const existing = getRowForLane(summary.laneId)
?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber);
...
update pull_requests
set lane_id = ?,That leaves the reused pr_id attached to the old queue/integration group, and later code still resolves workflow membership by pr_group_members.pr_id plus the cached member lane_id (getMergeContext, landQueueNext, and the GitHub snapshot). In practice, adopting/linking a PR that was previously tracked on lane A makes the new lane B PR still appear in—and potentially be acted on from—lane A's old group. When reusing a row for a different lane, clear or migrate its pr_group_members rows atomically before returning the reused pr_id.
| throw new Error( | ||
| `Failed to create pull request for "${headBranch}" → "${baseBranch}": ${msg}` | ||
| ); | ||
| const existingPr = /pull request already exists|already exists|validation failed/i.test(msg) |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
The duplicate-PR recovery path is keyed off a generic message match instead of a duplicate-specific error.
// apps/desktop/src/main/services/prs/prService.ts
const msg = err instanceof Error ? err.message : String(err);
const existingPr = /pull request already exists|already exists|validation failed/i.test(msg)
? await findExistingPrForBranch(repo, headBranch, baseBranch).catch((lookupError) => {GitHub returns 422 Validation Failed for unrelated create-PR errors too (for example invalid head/base data), so this branch can silently convert a real create failure into "success" by adopting whatever existing PR matches the branch. That leaves the lane bound to the wrong PR and hides the actual user error. Only trigger fallback for the duplicate-PR case, or parse the structured 422 error payload/status before adopting an existing PR.
| const existingPr = /pull request already exists|already exists|validation failed/i.test(msg) | |
| const existingPr = /pull request already exists/i.test(msg) |
|
@copilot review but do not make fixes |
| "startQueue", | ||
| ], | ||
| pr_summary: ["ensureSummary", "getSummary", "regenerateSummary"], | ||
| flow_policy: [ |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
This allowlist now exposes privileged flow-policy and Linear sync controls through run_ade_action, even though the same operations are already guarded behind callerCtx.role === "cto" in the dedicated tool handlers I verified at lines 4904-4983. ts // apps/ade-cli/src/adeRpcServer.ts flow_policy: [ "diffPolicyPaths", "getPolicy", "listRevisions", "normalizePolicy", "rollbackRevision", "savePolicy", The diff also adds linear_sync.runSyncNow/resolveQueueItem, linear_ingress.ensureRelayWebhook, and linear_routing.simulateRoute, but run_ade_action itself has no role check and is already callable from agent sessions (covered by the existing agent-path tests in @apps/ade-cli/src/adeRpcServer.test.ts). That means worker/agent callers can now mutate ADE's flow policy, drive sync runs, resolve queue items, and register the webhook through the generic action bridge, bypassing the intended CTO-only boundary. Remove these privileged methods from the dynamic allowlist or add equivalent role enforcement in run_ade_action for the affected domains.
| sections.push( | ||
| [ | ||
| "ADE TOOLING: Use ADE's action surface or the `ade` CLI for team collaboration commands when available.", | ||
| ADE_CLI_AGENT_GUIDANCE, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
buildWorkerBaseGuidance() now pushes ADE_CLI_AGENT_GUIDANCE in two unconditional sections, so the inspector prompt contains two identical ## ADE CLI blocks even though the actual worker prompt builder only injects that guidance once. That makes the inspector inaccurate for debugging and wastes prompt budget in the preview. Keep the first shared guidance block and remove it from the later worker-identity section. ```ts
// apps/desktop/src/main/services/orchestrator/promptInspector.ts
[
ADE_CLI_AGENT_GUIDANCE,
"Your worker identity (mission, run, step, attempt IDs) is automatically resolved — you don't need to pass IDs to observation tools.",
"Key actions available:",
| : null, | ||
| buildExecutionModeDirective(executionMode, managed.session.provider), | ||
| buildClaudeInteractionModeDirective(managed.session.interactionMode, managed.session.provider), | ||
| shouldInjectLaneDirective ? ADE_CLI_AGENT_GUIDANCE : null, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
For normal Claude sessions, buildClaudeV2SessionOpts() already appends ADE_CLI_AGENT_GUIDANCE to the session system prompt, and this new branch prepends the same block again into the first user message whenever the lane directive is injected. The result is a duplicated ## ADE CLI section on every first Claude turn in a lane-backed chat, which wastes context and over-weights the same instruction in a path that previously had only one copy. Limit this injection to providers that lack an equivalent persistent system prompt, or skip it for Claude. ```ts
// apps/desktop/src/main/services/chat/agentChatService.ts
composeLaunchDirectives(trimmed, [
shouldInjectLaneDirective ? buildLaneWorktreeDirective(...) : null,
buildExecutionModeDirective(executionMode, managed.session.provider),
buildClaudeInteractionModeDirective(managed.session.interactionMode, managed.session.provider),
shouldInjectLaneDirective ? ADE_CLI_AGENT_GUIDANCE : null,
])
| linear_sync: ["getDashboard", "getRunDetail", "listQueue", "resolveQueueItem", "runSyncNow"], | ||
| linear_ingress: ["ensureRelayWebhook", "getStatus", "listRecentEvents"], | ||
| linear_routing: ["simulateRoute"], | ||
| github: ["clearToken", "getRepoOrThrow", "getStatus", "setToken"], |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
This diff adds sensitive mutators to the generic ADE action allowlist, for example:
// apps/ade-cli/src/adeRpcServer.ts
github: ["clearToken", "getRepoOrThrow", "getStatus", "setToken"],
feedback: ["list", "prepareDraft", "submitPreparedDraft"],
usage: ["forceRefresh", "getUsageSnapshot", "poll", "start", "stop"],
budget: ["checkBudget", "getConfig", "getCumulativeUsage", "getGlobalCumulativeUsage", "recordUsage", "updateConfig"],
update: ["checkForUpdates", "dismissInstalledNotice", "getSnapshot", "quitAndInstall"],run_ade_action is a base tool with no role-specific gate, and listToolSpecsForSession includes all base tools for agent and even external sessions, so these newly allowlisted entries let untrusted callers overwrite stored GitHub/Linear credentials, run the packaged CLI installer / mutate process.env via ade_cli, or force quitAndInstall() from the generic RPC surface. That turns settings/update operations into agent-callable actions and violates the project requirement to enforce policy in code. Remove privileged mutators from the generic allowlist or add explicit role checks inside run_ade_action for sensitive domains/actions so only trusted UI/CTO paths can invoke them.
| function hasHelpFlag(args: string[]): boolean { | ||
| const terminatorIndex = args.indexOf("--"); | ||
| const searchable = terminatorIndex >= 0 ? args.slice(0, terminatorIndex) : args; | ||
| return searchable.includes("--help") || searchable.includes("-h"); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
hasHelpFlag() now short-circuits on any raw -h/--help token before the command-specific parser can consume it as a value. That breaks legitimate inputs such as ade chat send <session> --text --help or ade files write <path> --text --help, which previously would pass the literal string through readValue() but now return help instead of executing. Fix by making help detection aware of option arity/value positions, or by handling --help inside each command parser rather than scanning the raw argv up front.
// apps/ade-cli/src/cli.ts
function hasHelpFlag(args: string[]): boolean {
const terminatorIndex = args.indexOf("--");
const searchable = terminatorIndex >= 0 ? args.slice(0, terminatorIndex) : args;
return searchable.includes("--help") || searchable.includes("-h");
}| ctoStateService, | ||
| workerAgentService, | ||
| workerBudgetService, | ||
| prService: headlessLinearServices.prService, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
createAdeRuntime() widens AdeRuntime for new headless-only domains, but this return block still forwards only a subset of headlessLinearServices, so linearCredentialService, workerTaskSessionService, and workerHeartbeatService are dropped on the floor even though createHeadlessLinearServices() constructs them. adeRpcServer.ts publishes linear_credentials, worker_task_session, and worker_heartbeat actions directly from runtime.*Service and filters out null services, so these CLI capabilities remain unavailable in headless mode despite this PR's registration work. Wire the already-created services through the returned runtime object (and any other intended headless services) so the RPC layer can actually expose them.
// apps/ade-cli/src/bootstrap.ts
workerBudgetService,
prService: headlessLinearServices.prService,
fileService: headlessLinearServices.fileService,
flowPolicyService: headlessLinearServices.flowPolicyService,
linearDispatcherService: headlessLinearServices.linearDispatcherService,| // lane, so `preferred_integration_lane_id` stays unset. When merging into an existing lane, that | ||
| // lane id is stored here (and normally matches `integration_lane_id`). Inferring "adopted" from | ||
| // `preferred === integration` alone is unsafe because merge-into and integration ids can differ. | ||
| if (!preferredIntegrationLaneId) return "ade-created"; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
// apps/desktop/src/main/services/prs/prService.ts
const preferredIntegrationLaneId = asString(row.preferred_integration_lane_id).trim() || null;
if (!preferredIntegrationLaneId) return "ade-created";
return "adopted";This heuristic now treats any non-null preferred_integration_lane_id as proof that the integration lane was adopted, but commitIntegration() can persist a new preferredIntegrationLaneId even when it keeps an existing scratch integration_lane_id (existingIntegrationLaneId is used at lines 3844-3848, then the override is still written back at lines 3908-3911). After that row shape, cleanupIntegrationWorkflow() and deleteIntegrationProposal() call isAdeOwnedIntegrationLane() and will skip archiving/deleting the ADE-created scratch lane because origin is misreported as "adopted". Record explicit provenance, or at minimum avoid treating mismatched preferred/integration ids as adopted.
- Expand ADE CLI RPC, bootstrap, and command surface; add shared adeCliGuidance for agent prompts. - Wire desktop orchestrator, chat, CTO, and main for PR/CLI behavior. - Derive integration scratch lanes from missing merge-into id; avoid misclassifying adopted lanes in cleanup. Made-with: Cursor
When --project-root or ADE_PROJECT_ROOT fixes the project root, default workspaceRoot to that path unless ADE_WORKSPACE_ROOT/--workspace-root is set (fixes capy-ai review thread). Made-with: Cursor
- Resolve adeRpcServer/bootstrap/cli conflicts with adeActions registry + headless services. - prService: clear pr_group_members when PR row moves lanes; tighter duplicate-PR message match. - CLI: route computer/artifact/setting aliases; safer hasHelpFlag after value-taking flags. - Orchestrator: log augmented startup preview when ADE CLI guidance is prepended. - main.ts: drop duplicate AdeRuntime passthrough keys. - TerminalView test: real timers + waitFor for webgl fallback flake. Made-with: Cursor
arul28
force-pushed
the
ade/cli-prs-fixes-747d7096
branch
from
f2351b8 to
1a650f0
Compare
| "pr", | ||
| "tests", | ||
| "chat", | ||
| "keybindings", |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This change expands the list_ade_actions / run_ade_action schemas with domains such as:
// apps/ade-cli/src/adeRpcServer.ts
"keybindings",
"agent_tools",
"ade_cli",
"layout",
"tiling_tree",
"graph_state",but the actual ADE action source of truth in apps/desktop/src/main/services/adeActions/registry.ts still defines the older AdeActionDomain union, ADE_ACTION_ALLOWLIST, and getAdeActionDomainServices() map only through automations/issue. I verified that list_ade_actions and run_ade_action both read from that registry, so these newly documented domains will either return no actions or fail with Domain '...' is unavailable in this runtime. That means the main feature of this group is nonfunctional for every newly listed domain. Either register each new domain in the shared action registry (plus tests) or remove it from both CLI schemas until backend support exists.
| function hasHelpFlag(args: string[]): boolean { | ||
| const terminatorIndex = args.indexOf("--"); | ||
| const searchable = terminatorIndex >= 0 ? args.slice(0, terminatorIndex) : args; | ||
| const valueCarrierFlags = new Set([ |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
hasHelpFlag() now short-circuits to command help before any subcommand parser runs, but it only treats a small hard-coded set of preceding flags as value carriers. In this same file, several handlers consume other value-bearing flags that are missing from the exemption list — for example -q in files search/memory search, --command in tests run/shell start, --workspace, --thread, --stack, and --app — so inputs like ade files search -q --help or ade tests run --lane lane-1 --command --help will now print help instead of passing the literal value through.
// apps/ade-cli/src/cli.ts
const valueCarrierFlags = new Set([
"--text",
"--body",
"--title",
"--question",That is a behavioral regression introduced by the new pre-dispatch help scan. The fix is to derive help detection from the same option readers the command handlers use, or at minimum expand the exemption list to every value-taking flag supported by the CLI surface.
| paths: AdeRuntimePaths; | ||
| logger: Logger; | ||
| db: AdeDb; | ||
| keybindingsService?: ReturnType<typeof createKeybindingsService> | null; |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
@apps/ade-cli/src/bootstrap.ts widens AdeRuntime with new headless surfaces such as keybindingsService, onboardingService, usageTrackingService, and autoUpdateService, but createAdeRuntime() still never instantiates or assigns those services when it builds runtime, so a real CLI runtime lacks the objects that the new RPC/action surface expects. I verified that @apps/ade-cli/src/adeRpcServer.ts:4191-4198 resolves run_ade_action directly from the runtime and throws Domain '${domain}' is unavailable in this runtime. when the service is missing, while @apps/ade-cli/src/adeRpcServer.test.ts:3208-3213 and :3242-3248 now expect headless sessions to expose and execute domains like keybindings, onboarding, usage, and update; outside the mocked test fixture those capabilities stay unavailable. ```ts
// apps/ade-cli/src/bootstrap.ts
keybindingsService?: ReturnType | null;
onboardingService?: ReturnType | null;
usageTrackingService?: ReturnType | null;
autoUpdateService?: ReturnType | null;
| ?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber); | ||
| if (existing) { | ||
| if (existing.lane_id !== summary.laneId) { | ||
| db.run(`delete from pr_group_members where pr_id = ?`, [existing.id]); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
upsertRow now reuses an existing pull_requests row for the same GitHub PR and rewrites its lane_id, but on a lane handoff it only clears pr_group_members:
// apps/desktop/src/main/services/prs/prService.ts
const existing = getRowForLane(summary.laneId)
?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber);
if (existing.lane_id !== summary.laneId) {
db.run(`delete from pr_group_members where pr_id = ?`, [existing.id]);
}That is incomplete because integration_proposals.linked_pr_id and other workflow state still point at the reused pr_id; loadIntegrationWorkflowRows() later dereferences that linked_pr_id through getRow(...) to compute cleanup/history state. If lane B adopts lane A’s PR row, the old integration workflow silently starts following lane B’s PR status, which can mark the wrong workflow merged/closed and drive incorrect cleanup decisions. When reusing a PR row across lanes, clear or migrate the other pr_id-backed workflow rows as well, or avoid mutating a row that is already attached to another workflow.
- adeRpcServer / adeActions registry: role-gate CTO-only methods (linear_credentials
set/clear, github set/clear token, update.quitAndInstall, flow_policy.save/rollback,
linear_sync.runSyncNow/resolveQueueItem, linear_ingress.ensureRelayWebhook,
budget.updateConfig, feedback.submitPreparedDraft, usage.start/stop/forceRefresh)
so agent-role callers cannot invoke them via run_ade_action.
- layout.set / tiling_tree.set / graph_state.set reject calls with missing payload
key instead of persisting default {}/null, preventing accidental UI state wipe.
- create_pr_from_lane threads baseBranch through prService.draftDescription so
auto-drafted title/body describe the actual target branch.
- Integration lane provenance: only flag as "adopted" when
preferred_integration_lane_id === integration_lane_id; mismatched scratch lanes
stay "ade-created" so cleanup/delete works.
- promptInspector: dedup ADE_CLI_AGENT_GUIDANCE block.
- agentChatService: skip first-user-message guidance injection for Claude
sessions (system prompt already carries it).
- cli hasHelpFlag: expand valueCarrierFlags to cover all ~120 value-taking
long-flags so --help after a pending value flag is treated as the value.
Addresses review comments 3134159150, 3134159153, 3134159164, 3134196314,
3134196316, 3134196319, 3134196322, 3134196340. Fixes 3 failing adeRpcServer
tests around dynamic ADE actions listing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
| "tests", | ||
| "chat", | ||
| "keybindings", | ||
| "agent_tools", |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new list_ade_actions/run_ade_action schemas advertise domains such as agent_tools, ade_cli, sync, mission_preflight, worker_budget, and lane_env, but the shared ADE action registry still does not include those domains in AdeActionDomain or getAdeActionDomainServices (verified in @apps/desktop/src/main/services/adeActions/registry.ts). That means calls like ade actions list agent_tools --text are accepted by the tool schema yet return no actions, and run_ade_action on the same domain fails with Domain 'agent_tools' is unavailable, so the CLI now claims support for functionality it cannot actually expose. Align the schema enums with the registry/runtime map, or finish wiring those domains into the shared registry before documenting them here.
// apps/ade-cli/src/adeRpcServer.ts
"keybindings",
"agent_tools",
"ade_cli",
"dev_tools",
"ai",
| return { kind: "execute", label: `coordinator ${toolName}`, steps: [actionCallStep("result", toolName, collectGenericObjectArgs(args))] }; | ||
| } | ||
|
|
||
| const VALUE_CARRIER_FLAGS: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
hasHelpFlag() now relies on a hard-coded carrier list, but that list still does not match the actual parsers. In the new table
// apps/ade-cli/src/cli.ts
const VALUE_CARRIER_FLAGS: ReadonlySet<string> = new Set([
"-b", "-t",
"--additional-instructions", "--app", "--arg", "--arg-json", "--arg-value",
"--arg-value-json", "--args-list-json", "--attempt", "--attempt-id",several live next-token carriers are missing even though the command handlers consume them via readValue()/readIntOption(): -m (git commit, lane merge, git stash), -q (files search, memory search), --max-rounds / --rounds (prs path-to-merge, prs pipeline), --max-log-bytes, --max-prompt-chars, and --recent-limit. Because runCli() passes those tokens through parseCliArgs() unchanged into buildCliPlan(), inputs like ade git commit -m --help, ade files search -q --help, or ade prs path-to-merge pr-1 --max-rounds --help will still be misclassified as help requests instead of treating --help as the value for that option. Fix by deriving the carrier set from the same option metadata used by readValue() / readIntOption(), or at minimum add every missing value-taking alias and cover them with negative tests.
| linear_ingress: ["ensureRelayWebhook"], | ||
| budget: ["updateConfig"], | ||
| feedback: ["submitPreparedDraft"], | ||
| usage: ["start", "stop", "forceRefresh"], |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
ADE_ACTION_CTO_ONLY is meant to cover anything that “drives privileged polling”, but the new usage allowlist still exposes poll to every caller role. ts // apps/desktop/src/main/services/adeActions/registry.ts github: ["clearToken", "getRepoOrThrow", "getStatus", "setToken"], feedback: ["list", "prepareDraft", "submitPreparedDraft"], usage: ["forceRefresh", "getUsageSnapshot", "poll", "start", "stop"], I verified in @apps/desktop/src/main/services/usage/usageTrackingService.ts that poll() is the live collector which reads usage windows/costs and is the same routine called by start() and forceRefresh(). Because run_ade_action only checks ADE_ACTION_CTO_ONLY, an agent or orchestrator caller can still invoke usage.poll and trigger the privileged polling path the new gate was supposed to reserve for CTO callers. Add poll to ADE_ACTION_CTO_ONLY.usage (or stop exposing it) so all polling entrypoints are consistently gated.
| usage: ["start", "stop", "forceRefresh"], | |
| usage: ["start", "stop", "forceRefresh", "poll"], |
| const layout = rawLayout && typeof rawLayout === "object" && !Array.isArray(rawLayout) | ||
| ? clampDockLayout(rawLayout as Record<string, unknown>) | ||
| : {}; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new layout ADE action says callers should pass explicit null to clear, but the implementation currently treats any non-object payload as {} and writes that empty layout to the DB. ts // apps/desktop/src/main/services/adeActions/registry.ts const rawLayout = args.layout; const layout = rawLayout && typeof rawLayout === "object" && !Array.isArray(rawLayout) ? clampDockLayout(rawLayout as Record<string, unknown>) : {}; Since run_ade_action accepts untyped JSON, a malformed CLI call like layout: true, layout: 1, or an array will silently wipe the saved layout instead of rejecting invalid input. That is destructive behavior introduced by this bridge, not just numeric clamping. Reject non-object/non-null payloads before setJson, and keep null as the only clear signal.
| const layout = rawLayout && typeof rawLayout === "object" && !Array.isArray(rawLayout) | |
| ? clampDockLayout(rawLayout as Record<string, unknown>) | |
| : {}; | |
| if (rawLayout !== null && (typeof rawLayout !== "object" || Array.isArray(rawLayout))) throw new Error("Expected 'layout' to be an object or null."); | |
| const layout = rawLayout === null ? {} | |
| : clampDockLayout(rawLayout as Record<string, unknown>); |
| : null, | ||
| buildExecutionModeDirective(executionMode, managed.session.provider), | ||
| buildClaudeInteractionModeDirective(managed.session.interactionMode, managed.session.provider), | ||
| shouldInjectLaneDirective && !providerHasPersistentGuidance ? ADE_CLI_AGENT_GUIDANCE : null, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
ADE_CLI_AGENT_GUIDANCE is now coupled to lane-directive freshness instead of provider capability:
// apps/desktop/src/main/services/chat/agentChatService.ts
shouldInjectLaneDirective
? buildLaneWorktreeDirective({ ... })
: null,
...
shouldInjectLaneDirective && !providerHasPersistentGuidance ? ADE_CLI_AGENT_GUIDANCE : null,For Codex/OpenCode/Cursor, promptText is the only place this guidance is supplied: runTurn(), runCursorTurn(), and sendCodexMessage() forward args.promptText directly, and only Claude gets a persistent system prompt from buildClaudeV2SessionOpts(). After lastLaneDirectiveKey is persisted, subsequent non-Claude turns stop receiving the ADE CLI instructions entirely, which regresses the intended always-available guidance and makes those sessions more likely to report ADE tasks as blocked instead of trying ade first. Inject the shared guidance whenever the provider lacks a persistent copy, independent of whether a lane directive is being re-sent.
| shouldInjectLaneDirective && !providerHasPersistentGuidance ? ADE_CLI_AGENT_GUIDANCE : null, | |
| !providerHasPersistentGuidance ? ADE_CLI_AGENT_GUIDANCE : null, |
| memoryService, | ||
| ctoStateService, | ||
| workerAgentService, | ||
| workerBudgetService, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
createAdeRuntime() adds many new optional AdeRuntime slots (keybindingsService, onboardingService, feedbackReporterService, usageTrackingService, budgetCapService, autoUpdateService, etc.), but this return object still only wires a handful of additions. getAdeActionDomainServices() reads those runtime fields directly, and list_ade_actions / run_ade_action drop falsy services or throw Domain '<name>' is unavailable in this runtime, so the new CLI domains never become callable in the real headless bootstrap path.
// apps/ade-cli/src/bootstrap.ts
workerBudgetService,
githubService: headlessLinearServices.githubService as never,
workerTaskSessionService: headlessLinearServices.workerTaskSessionService,
workerHeartbeatService: headlessLinearServices.workerHeartbeatService,
linearCredentialService: headlessLinearServices.linearCredentialService as never,Either construct and assign the new headless-safe services here (for example createKeybindingsService({ db }) plus the other intended registrations) or remove their domains from the CLI action surface until they are actually wired.
| let title = asOptionalTrimmedString(toolArgs.title); | ||
| let body = typeof toolArgs.body === "string" ? toolArgs.body : null; | ||
| if (!title || body == null) { | ||
| const draft = await prSvc.draftDescription({ |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When create_pr_from_lane is called without an explicit baseBranch, the new draft flow asks prService.draftDescription to infer the PR body from lane.baseRef, but prService.createFromLane creates the actual PR against resolveStableLaneBaseBranch(...). Those are not equivalent for stacked lanes — @apps/desktop/src/shared/laneBaseResolution.test.ts documents a child lane whose stored baseRef is main while the stable PR base is the parent branch. That means this new path can generate a title/body describing changes against main and then immediately open the PR against the parent lane instead. Use the same stable-base resolution before drafting and pass that branch into draftDescription so the generated content matches the PR target.
// apps/ade-cli/src/adeRpcServer.ts
if (!title || body == null) {
const draft = await prSvc.draftDescription({
laneId,
...(baseBranch ? { baseBranch } : {}),
});| const now = nowIso(); | ||
| const existing = getRowForLane(summary.laneId); | ||
| const existing = getRowForLane(summary.laneId) | ||
| ?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
upsertRow() now adopts any existing row for the same repo/PR number before insert, then immediately strips its queue/workflow links if that row belongs to another lane.
// apps/desktop/src/main/services/prs/prService.ts
const existing = getRowForLane(summary.laneId)
?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber);
if (existing.lane_id !== summary.laneId) {
db.run(`delete from pr_group_members where pr_id = ?`, [existing.id]);
db.run(`update integration_proposals set linked_pr_id = null where linked_pr_id = ?`, [existing.id]);
}Because linkToLane() always flows through upsertRow(), linking a PR that is already attached to another active lane now silently reassigns lane_id, drops its pr_group_members, and detaches any integration workflow that referenced it. This is a behavior change from “link this lane to that PR” to “move that PR record away from the other lane,” which corrupts the original lane’s state. Restrict repo+number adoption to the duplicate-create recovery path, or require an explicit adoption flag plus source-lane safety checks before mutating lane_id.
| if (candidates.length === 0) return null; | ||
| const open = candidates.find((candidate) => { | ||
| const state = asString(candidate?.state).toLowerCase(); | ||
| return state === "open" || Boolean(candidate?.draft); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
findExistingPrForBranch() still treats draft: true as equivalent to an open PR.
// apps/desktop/src/main/services/prs/prService.ts
const open = candidates.find((candidate) => {
const state = asString(candidate?.state).toLowerCase();
return state === "open" || Boolean(candidate?.draft);
});GitHub leaves draft: true on closed draft PRs, so the duplicate-create recovery path can bind the lane to an abandoned closed draft instead of the newer open PR for the same branch. That makes later refresh/land operations target the wrong GitHub PR. Match only state === "open" here; open drafts already satisfy that condition.
| return state === "open" || Boolean(candidate?.draft); | |
| return state === "open"; |
| }; | ||
|
|
||
| const getRow = (prIdOrLocator: string): PullRequestRow | null => | ||
| getRowById(prIdOrLocator) ?? getRowByLocator(prIdOrLocator); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new lookup fallback makes every prId-taking read API accept PR locators, but the snapshot cache still persists the caller’s original string instead of the canonical row id.
// apps/desktop/src/main/services/prs/prService.ts
const getRow = (prIdOrLocator: string): PullRequestRow | null =>
getRowById(prIdOrLocator) ?? getRowByLocator(prIdOrLocator);
const requireRow = (prId: string): PullRequestRow => {Methods such as getStatus, getChecks, getComments, getReviews, getDetail, and getFiles resolve #123 / PR URLs through getRow(), then call upsertSnapshotRow({ prId, ... }) with that raw locator. pull_request_snapshots.pr_id references pull_requests(id), so the newly supported locator form succeeds for the read and then fails or miskeys the snapshot write because it is not a real PR row id. Canonicalize snapshot writes to row.id anywhere locator lookup is allowed, or keep locator support out of snapshot-producing methods.
- prService.findExistingPrForBranch: drop `|| Boolean(candidate?.draft)`. GitHub keeps draft=true on closed drafts, so the OR could prefer a closed draft over a later open PR. state==="open" is sufficient. - cli.ts VALUE_CARRIER_FLAGS: add -m, --max-rounds / --rounds, --max-log-bytes, --max-prompt-chars, --recent-limit; drop --text (global output flag, broke `lanes list --text --help`). - cli.test.ts: regression for `--text --help` ordering. - cli.ts findAdeManagedWorktreeRoot / findProjectRoots: canonicalize with fs.realpathSync.native so symlinked worktree paths still match. - coordinatorAgent.ts: drop ADE_CLI_AGENT_GUIDANCE injection — coordinator delegates to workers and has no terminal tool, so the guidance misled it. - main.ts buildAdeActionRuntimeForAutomations: forward keybindingsService, onboardingService, feedbackReporterService, usageTrackingService, budgetCapService, autoUpdateService so desktop automations can dispatch the new action domains. Addresses review comments 3134349765, 3134365534, 3134365536, 3134365659, 3134365660, 3134365668. 3134365535 already addressed in iter 3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
| ctoStateService, | ||
| workerAgentService, | ||
| workerBudgetService, | ||
| githubService: headlessLinearServices.githubService as never, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
@apps/ade-cli/src/bootstrap.ts now wires a few additional headless services into the returned runtime, but this assembly still never sets several newly advertised action services such as keybindingsService, onboardingService, feedbackReporterService, usageTrackingService, and budgetCapService.
// apps/ade-cli/src/bootstrap.ts
workerAgentService,
workerBudgetService,
githubService: headlessLinearServices.githubService as never,
workerTaskSessionService: headlessLinearServices.workerTaskSessionService,
workerHeartbeatService: headlessLinearServices.workerHeartbeatService,That is a real behavior bug in headless mode: when the desktop socket is absent, cli.ts falls back to createAdeRuntime(), getAdeActionDomainServices(runtime) reads these fields directly, and run_ade_action throws Domain '<x>' is unavailable in this runtime. for missing services. Desktop already constructs keybindings, onboarding, feedback, usage, and budget from dependencies this function already has (db, logger, projectConfigService, laneService, aiIntegrationService, and the headless GitHub service), so the fix is to instantiate and attach the headless-safe services here as well, or stop exposing those domains from the headless runtime until they are actually wired.
| `select ${PR_COLUMNS} | ||
| from pull_requests | ||
| where project_id = ? | ||
| and github_pr_number = ? |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
The new numeric locator path resolves #123 against pull_requests by project_id + github_pr_number only, even though this service now stores PRs from multiple repositories (linkToLane accepts full PR URLs, and the GitHub snapshot code explicitly tracks externalPullRequests). ts // apps/desktop/src/main/services/prs/prService.ts const getRowByNumber = (prNumber: number): PullRequestRow | null => db.get<PullRequestRow>( `select ${PR_COLUMNS} from pull_requests where project_id = ? and github_pr_number = ? If a project has both ownerA/repo1#123 and ownerB/repo2#123, any API that now accepts prId="#123" can pick whichever row was updated last and then operate on the wrong PR (open, land, retarget, comment, etc.). Scope bare-number lookups to the current repo, or reject them when more than one repo matches, instead of doing a project-wide lookup.
| const segments = resolved.split(path.sep); | ||
| for (let index = segments.length - 2; index >= 0; index -= 1) { | ||
| if (segments[index] !== ".ade" || segments[index + 1] !== "worktrees") continue; | ||
| const projectRoot = segments.slice(0, index).join(path.sep) || path.sep; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When the ADE project itself lives at a Windows drive root (for example C:\), splitting the resolved path and rejoining segments.slice(0, index) produces C: instead of C:\. The next line then runs path.resolve(projectRoot), and Node documents that path.resolve('C:') is drive-relative rather than the drive root. That makes the CLI derive the socket path and headless runtime from the caller’s current-directory-on-drive instead of the actual ADE project, so invocations from C:\.ade\worktrees\<lane> can fail to connect or open the wrong database. Use the parsed root when .ade is immediately under the filesystem root.
// apps/ade-cli/src/cli.ts
const segments = resolved.split(path.sep);
for (let index = segments.length - 2; index >= 0; index -= 1) {
if (segments[index] !== ".ade" || segments[index + 1] !== "worktrees") continue;
const projectRoot = segments.slice(0, index).join(path.sep) || path.sep;| const projectRoot = segments.slice(0, index).join(path.sep) || path.sep; | |
| const projectRoot = index === 1 ? path.parse(resolved).root : segments.slice(0, index).join(path.sep) || path.sep; |
| usageTrackingService, | ||
| budgetCapService, | ||
| sessionDeltaService, | ||
| autoUpdateService, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
@apps/desktop/src/main/main.ts adds autoUpdateService to the rpcRuntime object before the global updater is initialized, so the embedded socket RPC server permanently captures null for the new update ADE-action domain. ts // apps/desktop/src/main/main.ts feedbackReporterService, usageTrackingService, budgetCapService, sessionDeltaService, autoUpdateService, issueInventoryService, rpcRuntime is passed to createAdeRpcRequestHandler() immediately afterward, but autoUpdateService is only assigned later (let autoUpdateService = null near line 4154, then autoUpdateService = createAutoUpdateService(...) near line 4409). Since getAdeActionDomainServices() exposes update from runtime.autoUpdateService, desktop socket clients will never see or be able to call update.getSnapshot, update.checkForUpdates, etc. even after startup finishes. Initialize the updater before constructing rpcRuntime, or patch rpcRuntime.autoUpdateService after creating the service so the embedded RPC runtime sees the live instance.
| // 3) second timer flush lets the post-import code run | ||
| for (let i = 0; i < 100; i++) { | ||
| // `await import("@xterm/addon-webgl")` may not settle under Vi's fake timers on CI shards. | ||
| vi.useRealTimers(); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This test now exits as soon as the renderer snapshot flips to "dom", but that is earlier than TerminalView's parked-runtime background work finishing. In @apps/desktop/src/renderer/components/terminals/TerminalView.tsx, unmounting a live terminal only parks it; it does not cancel settleTimer1, settleTimer2, hydrateTimer, or hydrateRetryTimer, and startHydration() will still call window.ade.sessions.readTranscriptTail(...) later. Because this case switches to real timers, those native timers can fire during the next test and increment that next test's fresh mocks, making the suite flaky again. The previous runAllTimersAsync loop drained this background work before the test ended. Either wait for the hydration/settle timers to complete as part of this test, or explicitly force the runtime to dispose before restoring fake timers.
// apps/desktop/src/renderer/components/terminals/TerminalView.test.tsx
vi.useRealTimers();
...
await waitFor(
() => {
const runtime = getTerminalRuntimeSnapshot("session-dom");
|
@copilot review but do not make fixes |
| throw new Error("Missing required 'layout' object. Pass an explicit null to clear."); | ||
| } | ||
| const rawLayout = args.layout; | ||
| const layout = rawLayout && typeof rawLayout === "object" && !Array.isArray(rawLayout) |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
layout.set claims callers can pass null to clear, but the new adapter coerces every non-object payload into {} and writes it to the DB instead of rejecting it. In @apps/desktop/src/main/services/adeActions/registry.ts:
const rawLayout = args.layout;
const layout = rawLayout && typeof rawLayout === "object" && !Array.isArray(rawLayout)
? clampDockLayout(rawLayout as Record<string, unknown>)
: {};
runtime.db.setJson(`dock_layout:${layoutId}`, layout);That matters because useDockLayout treats any truthy saved value as authoritative and does setLayout(saved), so a malformed CLI call like layout.set --input-json '{"layoutId":"main","layout":null}' replaces the user's persisted panel arrangement with an empty layout on the next load. Reject non-object payloads here (and handle explicit clears deliberately) instead of silently persisting {}.
| : null, | ||
| buildExecutionModeDirective(executionMode, managed.session.provider), | ||
| buildClaudeInteractionModeDirective(managed.session.interactionMode, managed.session.provider), | ||
| shouldInjectLaneDirective && !providerHasPersistentGuidance ? ADE_CLI_AGENT_GUIDANCE : null, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new guidance injection is coupled to shouldInjectLaneDirective, but lastLaneDirectiveKey is persisted and restored across session resume, so resumed Codex/OpenCode/Cursor sessions in the same lane will evaluate that flag to false and never receive ADE_CLI_AGENT_GUIDANCE at all. I verified that only Claude has a persistent prompt path (buildClaudeV2SessionOpts), while OpenCode/Codex send whatever lands in promptText for each turn. That means the exact resumed-session case this PR is trying to standardize still misses the ade instructions for every non-Claude provider.
// apps/desktop/src/main/services/chat/agentChatService.ts
const laneDirectiveKey = executionContext.laneDirectiveKey;
const shouldInjectLaneDirective = laneDirectiveKey != null && managed.lastLaneDirectiveKey !== laneDirectiveKey;
const providerHasPersistentGuidance = managed.session.provider === "claude";
...
shouldInjectLaneDirective && !providerHasPersistentGuidance ? ADE_CLI_AGENT_GUIDANCE : null,Decouple ADE guidance delivery from lane-directive churn: either track a separate "guidance delivered" bit for non-Claude sessions, or inject the guidance on the first non-Claude user turn after runtime/session resume even when the lane binding itself does not need reinjection.
| expect(allDomains.structuredContent.actions.some((entry: { domain: string }) => entry.domain === "worker_agent")).toBe(true); | ||
| expect(allDomains.structuredContent.actions.some((entry: { domain: string }) => entry.domain === "computer_use_artifacts")).toBe(true); | ||
| expect(allDomains.structuredContent.actions.some((entry: { domain: string }) => entry.domain === "operation")).toBe(true); | ||
| expect(allDomains.structuredContent.actions.some((entry: { domain: string }) => entry.domain === "keybindings")).toBe(true); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
These new assertions assume the headless ade runtime exposes desktop-only domains such as keybindings, onboarding, usage, and update, but createAdeRuntime() is the actual runtime used by ade headless mode and it still does not populate keybindingsService, onboardingService, usageTrackingService, or autoUpdateService. As a result, list_ade_actions in headless mode will not return these domains and run_ade_action will fail with Domain '<name>' is unavailable in this runtime. even though this patch now treats them as part of the expanded CLI surface. The test therefore gives false confidence that the feature works end-to-end. Either wire those services into apps/ade-cli/src/bootstrap.ts for headless mode, or scope these expectations to the desktop-socket runtime only.
// apps/ade-cli/src/adeRpcServer.test.ts
expect(allDomains.structuredContent.actions.some((entry: { domain: string }) => entry.domain === "keybindings")).toBe(true);
expect(allDomains.structuredContent.actions.some((entry: { domain: string }) => entry.domain === "onboarding")).toBe(true);
expect(allDomains.structuredContent.actions.some((entry: { domain: string }) => entry.domain === "usage")).toBe(true);
expect(allDomains.structuredContent.actions.some((entry: { domain: string }) => entry.domain === "update")).toBe(true);| "--branch-name", "--branch-ref", "--category", "--color", "--cols", | ||
| "--command", "--comment", "--comment-id", "--commit", "--compare-ref", | ||
| "--compare-to", "--content", "--context-file", "--cwd", "--data", | ||
| "--delete-source", "--delete-source-lane", "--depth", "--desc", |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
VALUE_CARRIER_FLAGS is now used by hasHelpFlag() to decide whether a --help token is really a help request or just the value for the previous flag, but this set now includes flags that are consumed with readFlag() rather than readValue(). For example, --delete-source, --include-ignored, and --unresolved are boolean switches in the lane/files/PR builders, so commands like ade lanes merge lane-1 --delete-source --help and ade files tree --include-ignored --help will skip help and fall through into normal execution. Remove boolean-only switches from VALUE_CARRIER_FLAGS (or derive this list from actual readValue consumers) so --help is only suppressed after flags that really take a value.
// apps/ade-cli/src/cli.ts
"--delete-source", "--delete-source-lane", "--depth", "--desc",
"--description", "--domain", "--duration-sec", "--enabled", "--event",
"--from-file", "--group", "--group-id", "--head", "--icon", "--id",
"--include-ignored", "--input", "--input-json", "--instructions",| gitService, | ||
| diffService, | ||
| missionService, | ||
| missionBudgetService, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This hunk only wires a subset of the newly-added AdeRuntime service slots, and createAdeRuntime never assigns keybindingsService, onboardingService, usageTrackingService, or autoUpdateService anywhere. ```ts
// apps/ade-cli/src/bootstrap.ts
missionBudgetService,
ptyService,
testService,
aiIntegrationService,
agentChatService,
That matters because `getAdeActionDomainServices()` maps those domains directly from the runtime object, and `run_ade_action` throws `Domain '<x>' is unavailable in this runtime` when a slot is missing. This PR also adds those domains to `ADE_ACTION_DOMAIN_NAMES`, and the new RPC tests expect them to be callable via a handcrafted fixture, so real headless sessions created through `createAdeRuntime` will fail for the newly advertised actions instead of matching the desktop/socket runtime. Wire the same services into headless bootstrap (or stop advertising those domains there) so the CLI contract stays in sync with the actual runtime.
| if (parsed.owner && parsed.repo) { | ||
| return getRowForRepoPr(parsed.owner, parsed.repo, parsed.number); | ||
| } | ||
| return getRowByNumber(parsed.number); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new locator resolver treats "123"/"#123" as a project-wide lookup and falls back to getRowByNumber, which has no repo filter and simply returns the most recently updated matching row. This service already stores PRs for multiple repositories (repo_owner/repo_name are first-class fields, and the GitHub snapshot logic handles external repos), so in a project tracking foo/api#123 and foo/web#123, any prId API that now accepts a numeric locator can act on the wrong PR. Reject repo-less numeric locators when more than one matching repo exists, or require owner/repo context for number-based lookup.
// apps/desktop/src/main/services/prs/prService.ts
if (parsed.owner && parsed.repo) {
return getRowForRepoPr(parsed.owner, parsed.repo, parsed.number);
}
return getRowByNumber(parsed.number);| const segments = resolved.split(path.sep); | ||
| for (let index = segments.length - 2; index >= 0; index -= 1) { | ||
| if (segments[index] !== ".ade" || segments[index + 1] !== "worktrees") continue; | ||
| const projectRoot = segments.slice(0, index).join(path.sep) || path.sep; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
findAdeManagedWorktreeRoot() rebuilds the inferred root by split(path.sep) / join(path.sep), which drops the trailing separator for drive-root projects. In a checkout rooted at C:\, segments.slice(0, index).join(path.sep) becomes "C:", and the later path.resolve(projectRoot) is volume-relative on Windows rather than the absolute drive root. That means invoking ade from C:\.ade\worktrees\<lane>\... can resolve projectRoot to the current directory on drive C: instead of C:\, so socket/runtime initialization points at the wrong .ade tree and the new auto-discovery fails for exactly the drive-root case this change is meant to support. Rebuild the path with the parsed absolute root (path.parse(resolved).root) or another root-preserving join strategy, and add a regression test for a C:\.ade\worktrees\... worktree.
// apps/ade-cli/src/cli.ts
const segments = resolved.split(path.sep);
for (let index = segments.length - 2; index >= 0; index -= 1) {
if (segments[index] !== ".ade" || segments[index + 1] !== "worktrees") continue;
const projectRoot = segments.slice(0, index).join(path.sep) || path.sep;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/main.ts (1)
3369-3407:⚠️ Potential issue | 🟠 MajorWire
linearCredentialServiceinto the automation ADE runtime.
getAdeActionDomainServices()resolves thelinear_credentialsdomain fromruntime.linearCredentialService, butbuildAdeActionRuntimeForAutomations()never supplies it. That leaves automationade-actionsteps unable to uselinear_credentialseven though the desktop RPC runtime does expose it.Suggested fix
flowPolicyService, + linearCredentialService, linearDispatcherService, linearIssueTracker, linearSyncService,🤖 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 3369 - 3407, buildAdeActionRuntimeForAutomations is not including linearCredentialService so getAdeActionDomainServices (which reads runtime.linearCredentialService for the linear_credentials domain) gets undefined; fix by adding linearCredentialService to the returned object in buildAdeActionRuntimeForAutomations so the runtime includes runtime.linearCredentialService (i.e., include a property named linearCredentialService alongside the other services in the returned AdeRuntime object) to ensure ade-action automation steps can access linear_credentials.
♻️ Duplicate comments (11)
apps/desktop/src/main/services/adeActions/registry.ts (2)
67-83:⚠️ Potential issue | 🟠 MajorRole gate bypass:
usage.pollis allowlisted but not CTO-only.
ADE_ACTION_CTO_ONLY.usagecoversstart,stop,forceRefresh, butusage.pollremains inADE_ACTION_ALLOWLIST.usageat Line 321 without a matching CTO gate. Sincerun_ade_actionenforces privilege viaADE_ACTION_CTO_ONLYonly, anyagent/orchestratorcaller can invokeusage.polland trigger the same privileged collection path the gate was introduced to protect. Previously verified thatpoll()is the live collector shared withstart()/forceRefresh().🛠️ Proposed fix
- usage: ["start", "stop", "forceRefresh"], + usage: ["start", "stop", "forceRefresh", "poll"],Also applies to: 321-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/adeActions/registry.ts` around lines 67 - 83, The allowlist contains usage.poll without CTO gating, so update ADE_ACTION_CTO_ONLY to include "poll" under the usage key to match the privileged actions; specifically, add "poll" alongside "start", "stop", and "forceRefresh" in ADE_ACTION_CTO_ONLY (so run_ade_action will properly enforce CTO-only callers for poll(), which shares the collector with start()/forceRefresh()); ensure ADE_ACTION_ALLOWLIST.usage still contains "poll" if needed but the CTO-only gate must be added in ADE_ACTION_CTO_ONLY to close the privilege gap.
480-492:⚠️ Potential issue | 🟠 Major
layout.setsilently destroys saved layout on malformed payloads.If
args.layoutis anything other than a plain object (e.g.,true,42,"foo", or an array), the ternary falls through to{}andruntime.db.setJson("dock_layout:" + layoutId, {})persists an empty layout. Becauserun_ade_actionaccepts untyped JSON, a single bad CLI/RPC call silently wipes a real user’s saved dock layout instead of failing loudly. The adjacenttiling_treeandgraph_statesetters explicitly reject a missing key;layoutshould similarly reject invalid shapes and reservenullas the only clear signal.🛠️ Proposed fix
const rawLayout = args.layout; - const layout = rawLayout && typeof rawLayout === "object" && !Array.isArray(rawLayout) - ? clampDockLayout(rawLayout as Record<string, unknown>) - : {}; + if (rawLayout !== null && (typeof rawLayout !== "object" || Array.isArray(rawLayout))) { + throw new Error("Expected 'layout' to be an object or null."); + } + const layout = rawLayout === null + ? {} + : clampDockLayout(rawLayout as Record<string, unknown>); runtime.db.setJson(`dock_layout:${layoutId}`, layout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/adeActions/registry.ts` around lines 480 - 492, The layout.set handler currently treats any non-plain-object as {} and overwrites saved layouts; change set (in the layout action) to explicitly validate args.layout: allow only null (to clear) or a plain object (non-array, typeof "object"); if args.layout is missing or is any other type (primitive or array) throw an Error (message: "Invalid 'layout' — pass an explicit null to clear or a plain object") instead of falling through to {}. After validation, call clampDockLayout on the plain object and persist via runtime.db.setJson(`dock_layout:${layoutId}`, layout) as before; keep requireNonEmptyString(layoutId) and the existing error text for missing layout key but tighten the type check.apps/ade-cli/src/cli.ts (2)
1860-1887:⚠️ Potential issue | 🟡 Minor
-cis missing fromVALUE_CARRIER_FLAGS.Both
buildShellPlan(Line 1458) andbuildTestsPlan(Line 1502) consume-cviareadValue(args, ["--command", "-c"]), but-cis absent from the new carrier list. As a result,ade shell start --lane l1 -c --helpandade tests run --lane l1 -c --helpare short-circuited to command help instead of passing the literal--helpthrough to--command. The-m,-q,-b,-tshort-form peers are already listed, so this is a straightforward gap.🛠️ Proposed fix
const VALUE_CARRIER_FLAGS: ReadonlySet<string> = new Set([ - "-b", "-m", "-q", "-t", + "-b", "-c", "-m", "-q", "-t",Related: the root cause (hard-coded carrier list that must stay in lockstep with every
readValue/readIntOptioncall site) was flagged previously. Deriving the set from the same metadata the parsers use would avoid this recurring drift.🤖 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 1860 - 1887, VALUE_CARRIER_FLAGS is missing the short flag "-c", which breaks buildShellPlan and buildTestsPlan because they call readValue(args, ["--command", "-c"]); update the ReadonlySet VALUE_CARRIER_FLAGS to include the string "-c" alongside the other short-form flags (e.g., "-b","-m","-q","-t") so that readValue can forward "-c" values correctly; after adding "-c", run the related shell/tests paths and consider adding a TODO to derive this set from the parser metadata to avoid future drift.
2009-2017:⚠️ Potential issue | 🟡 MinorWindows drive-root join still yields a drive-relative path.
When the ADE project sits at a drive root (e.g.,
C:\.ade\worktrees\<lane>),resolved.split(path.sep)produces["C:", ".ade", "worktrees", "<lane>", ...]. Atindex === 1,segments.slice(0, 1).join(path.sep)is"C:", which Node treats as drive-relative (current directory on driveC:), not the drive root"C:\". Downstreampath.resolve(projectRoot)then anchors to the process cwd-on-drive rather than the actual project. The symlink canonicalization added viafs.realpathSync.nativeis a good fix for the other past concern, but this one remains.🛠️ Proposed fix
- const projectRoot = segments.slice(0, index).join(path.sep) || path.sep; + const projectRoot = index === 1 + ? path.parse(resolved).root + : segments.slice(0, index).join(path.sep) || path.sep;🤖 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 2009 - 2017, The loop builds projectRoot/workspaceRoot from segments but on Windows a drive root becomes "C:" (drive-relative) which causes path.resolve(projectRoot) to resolve relative to the current directory on that drive; detect Windows drive-only tokens (e.g., projectRoot matching /^[A-Za-z]:$/) and append the path separator before using projectRoot/workspaceRoot (or use path.join(projectRoot, path.sep)) so that projectRoot becomes "C:\\" (true drive root) before calling fs.existsSync and path.resolve; apply the same normalization for workspaceRoot and any checks that use projectRoot.apps/desktop/src/main/main.ts (1)
3176-3249:⚠️ Potential issue | 🟠 MajorMake
autoUpdateServicelive in the socket RPC runtime.Line 3245 snapshots
autoUpdateServicewhile it is stillnull, and the RPC handler then keeps that stale value for the lifetime of the socket server. Desktop socket clients will never see theupdateADE-action domain even after the updater is created later in startup.Suggested fix
const rpcRuntime: AdeRuntime = { projectRoot, workspaceRoot: projectRoot, projectId, project, @@ - autoUpdateService, + get autoUpdateService() { + return autoUpdateService; + }, issueInventoryService, eventBuffer: rpcEventBuffer, dispose: () => {}, // desktop manages service lifecycle };🤖 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 3176 - 3249, The RPC runtime currently captures a null snapshot of autoUpdateService; instead ensure rpcRuntime uses the real service instance by assigning rpcRuntime.autoUpdateService = autoUpdateService after the updater is initialized (or define a live getter on rpcRuntime that returns the current autoUpdateService variable). Locate the rpcRuntime object and the autoUpdateService variable and either (a) remove autoUpdateService from the static literal and set rpcRuntime.autoUpdateService = autoUpdateService once the updater is created, or (b) use Object.defineProperty(rpcRuntime, "autoUpdateService", { get: () => autoUpdateService }) so socket RPC handlers always see the live updater instance.apps/ade-cli/src/bootstrap.ts (1)
526-582:⚠️ Potential issue | 🟠 MajorHeadless
AdeRuntimeis still missing several newly advertised action services.This runtime still never assigns
keybindingsService,onboardingService,feedbackReporterService,usageTrackingService,budgetCapService, orautoUpdateService. SincegetAdeActionDomainServices(runtime)reads those slots directly, headlessade actionswill keep rejecting those domains even though the desktop socket runtime now wires them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/bootstrap.ts` around lines 526 - 582, The headless AdeRuntime object is missing the newly required action services (keybindingsService, onboardingService, feedbackReporterService, usageTrackingService, budgetCapService, autoUpdateService) which causes getAdeActionDomainServices(runtime) to reject those domains; add those properties to the runtime object and wire them to the appropriate headless implementations (e.g., headlessLinearServices.keybindingsService, headlessLinearServices.onboardingService, etc.) or provide lightweight/no-op fallbacks from the same service factory, and ensure any added services are also disposed in the dispose() swallow block (call their dispose/disposeAll if available).apps/desktop/src/main/services/chat/agentChatService.ts (1)
10918-10930:⚠️ Potential issue | 🟠 MajorNon-Claude guidance is still incorrectly tied to lane-directive refresh.
On Line 10929, non-Claude providers only get
ADE_CLI_AGENT_GUIDANCEwhenshouldInjectLaneDirectiveis true. After lane directive persistence, guidance disappears on subsequent turns.Suggested fix
- shouldInjectLaneDirective && !providerHasPersistentGuidance ? ADE_CLI_AGENT_GUIDANCE : null, + !providerHasPersistentGuidance ? ADE_CLI_AGENT_GUIDANCE : null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 10918 - 10930, The ADE_CLI_AGENT_GUIDANCE entry is currently gated by shouldInjectLaneDirective so non-Claude providers lose guidance after lane persistence; change the condition that adds ADE_CLI_AGENT_GUIDANCE in the composeLaunchDirectives array so it does not depend on shouldInjectLaneDirective but instead checks that providerHasPersistentGuidance is false and the provider is not Claude (use managed.session.provider or existing buildClaudeInteractionModeDirective/interactionMode to detect Claude); i.e., add ADE_CLI_AGENT_GUIDANCE when !providerHasPersistentGuidance && managed.session.provider !== 'claude' (or equivalent), leaving the lane directive inclusion unchanged.apps/ade-cli/src/adeRpcServer.ts (1)
5413-5419:⚠️ Potential issue | 🟠 MajorDraft the PR text against the same base branch that will actually be used.
When
baseBranchis omitted, Line 5414 still asksdraftDescriptionto infer the diff on its own, whilecreateFromLanecan resolve a different stable base for stacked lanes. That can generate a title/body for one diff and then open the PR against another branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/adeRpcServer.ts` around lines 5413 - 5419, When baseBranch is omitted, prSvc.draftDescription is currently asked to infer the diff itself which can mismatch the stable base resolution used later by createFromLane; instead resolve the exact base branch that will be used (using the same resolver/helper or logic createFromLane uses) into a variable (e.g., resolvedBaseBranch) and pass that resolvedBaseBranch into prSvc.draftDescription({ laneId, ...(resolvedBaseBranch ? { baseBranch: resolvedBaseBranch } : {})}) so the drafted title/body match the actual PR base; update references around baseBranch, prSvc.draftDescription, createFromLane, laneId, title and body accordingly.apps/desktop/src/main/services/prs/prService.ts (3)
4917-4940:⚠️ Potential issue | 🟠 MajorCanonicalize snapshot writes before accepting PR locators.
These methods now resolve
prIdthroughgetRow()/requireRow(), so callers can pass#123or a PR URL. ButupsertSnapshotRow()still stores the raw argument aspull_request_snapshots.pr_id, which should referencepull_requests.id. Calls likegetStatus("#123")will mis-key or fail the snapshot write. Use the resolved row id for every snapshot upsert.💡 Minimal fix
async getStatus(prId: string): Promise<PrStatus> { const row = getRow(prId); if (!row) throw new Error(`PR not found: ${prId}`); const status = await computeStatus(rowToSummary(row)); - upsertSnapshotRow({ prId, status }); + upsertSnapshotRow({ prId: row.id, status }); return status; }, async getChecks(prId: string): Promise<PrCheck[]> { - const checks = await getChecks(prId); - upsertSnapshotRow({ prId, checks }); + const row = requireRow(prId); + const checks = await getChecks(row.id); + upsertSnapshotRow({ prId: row.id, checks }); return checks; },Apply the same canonicalization to
getComments,getReviews,getDetail, andgetFiles.Also applies to: 5110-5119
732-755:⚠️ Potential issue | 🟠 MajorReject bare
#123locators when more than one repo can match.
getRowByNumber()is project-wide, but this service now tracks PRs from multiple repositories. If two repos both have PR 123, any API that accepts a bare number can refresh, comment on, or land the wrong PR based on recency. Scope numeric locators to the active repo, or fail when the lookup is ambiguous.🤖 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 732 - 755, getRowByLocator currently resolves bare numeric locators via getRowByNumber which searches the whole project and can return the wrong PR if multiple repos in the project have the same PR number; change the logic so that when parsePrLocator returns only a number (no owner/repo) you first check for ambiguity by counting matching rows for that projectId and number (use the same query shape as getRowByNumber), and if more than one match exists either scope the lookup to the active repo (call getRowForRepoPr(activeOwner, activeRepo, parsed.number)) or return null/fail to avoid ambiguous resolution; update getRowByLocator to use parsePrLocator, getRowForRepoPr, and projectId accordingly and ensure ambiguous cases are rejected instead of returning the most-recent row.
1157-1210:⚠️ Potential issue | 🔴 CriticalDon't let
upsertRow()silently steal a PR from another lane.The repo+PR fallback now runs for every upsert, not just duplicate-create recovery. If another lane already owns that PR row, this path deletes
pr_group_members, nullsintegration_proposals.linked_pr_id, and rewriteslane_idwith no explicit handoff. That can break an active queue/integration workflow on the original lane. Keep repo+number adoption behind an explicit lane-transfer flow, or fail when the matched row belongs to a different lane.🤖 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 1157 - 1210, The upsertRow() currently uses a fallback getRowForRepoPr() and will unconditionally reassign lane ownership (deleting pr_group_members and nulling integration_proposals) which can steal an active PR from another lane; fix by splitting the lookup into two variables (e.g. const laneRow = getRowForLane(...); const repoRow = getRowForRepoPr(...); const existing = laneRow ?? repoRow) and if existing came from the repoRow fallback (laneRow is null) and existing.lane_id !== summary.laneId then do not perform the destructive handoff—either return existing.id unchanged or throw/return an error—so lane transfers only happen via an explicit transfer flow instead of silently rewriting lane_id in upsertRow().
🧹 Nitpick comments (3)
apps/ade-cli/src/cli.test.ts (1)
387-401: Temp directory frommkdtempSyncleaks after the test.
fs.mkdtempSynccreates a real temp directory but there is noafterEach/try..finallythat removes it. Over many local/CI runs this accumulates underos.tmpdir(). A smallfs.rmSync(rawRoot, { recursive: true, force: true })in afinally(or anafterEachscoped to this test) keeps the filesystem clean without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.test.ts` around lines 387 - 401, This test creates a real temp directory via rawRoot = fs.mkdtempSync(...) but never removes it; wrap the test body in a try/finally (or add an afterEach for this spec) and call fs.rmSync(rawRoot, { recursive: true, force: true }) in the finally/afterEach to ensure the temp directory is removed; reference the rawRoot variable created at the top of the test (and you can use root = fs.realpathSync.native(rawRoot) as before) so cleanup removes the actual on-disk directory created by mkdtempSync.apps/desktop/src/main/services/orchestrator/baseOrchestratorAdapter.ts (1)
798-799:startupCommandPreviewloses signal now that guidance is prepended.
ADE_CLI_AGENT_GUIDANCEis ~400+ characters, sooverridePrompt.slice(0, 320)will now always return the same guidance prefix for every override, making the stored preview useless for distinguishing which startup command was actually applied. Similarly,promptLengthnow counts the guidance prefix, so values aren't comparable against historical rows.Consider deriving the preview from
startupCommandOverride(the actual override) while keepingpromptLengthon the combinedoverridePrompt, or vice-versa, so the metadata keeps its debug value.♻️ Suggested adjustment
metadata: { adapterKind: executorKind, startupCommandOverride: true, promptLength: overridePrompt.length, - startupCommandPreview: overridePrompt.slice(0, 320) + startupCommandPreview: startupCommandOverride.slice(0, 320) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/baseOrchestratorAdapter.ts` around lines 798 - 799, The stored preview currently slices overridePrompt (which includes ADE_CLI_AGENT_GUIDANCE) causing every preview to show the same guidance; change startupCommandPreview to be derived from startupCommandOverride (e.g., use startupCommandOverride.slice(0, 320)) so the preview reflects the actual override, while keeping promptLength calculated from overridePrompt (to include the full combined length); alternatively, if you prefer promptLength to exclude guidance, compute promptLength from startupCommandOverride and keep startupCommandPreview from overridePrompt—update the assignments around promptLength, startupCommandPreview, overridePrompt, and startupCommandOverride to reflect the chosen approach.apps/ade-cli/src/adeRpcServer.test.ts (1)
3270-3283: Consider also covering thebaseBranch-omitted draft path.Per the PR summary,
create_pr_from_lanenow treats bothtitleandbaseBranchas optional (with auto-draft/inference). This test only exercises the title/body-omitted case while still passingbaseBranch: "main". Adding a second assertion that omitsbaseBranch(verifying it's either inferred by the server or forwarded as-is todraftDescription/createFromLane) would lock in the full optional-arg contract and prevent silent regressions on base-branch resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/adeRpcServer.test.ts` around lines 3270 - 3283, Add a second test invocation of callTool(handler, "create_pr_from_lane", { laneId: "lane-1" }) that omits baseBranch and assert the result and service calls mirror the optional-arg contract: verify drafted?.isError is undefined, that fixture.runtime.prService.draftDescription was called for the lane without a baseBranch (e.g., calledWith({ laneId: "lane-1" }) or with baseBranch undefined), and that fixture.runtime.prService.createFromLane was lastCalledWith an object containing laneId "lane-1" and either an inferred baseBranch or baseBranch undefined (match the server behavior you expect) so the test covers the baseBranch-omitted path for create_pr_from_lane.
🤖 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/ade-cli/src/adeRpcServer.ts`:
- Around line 4098-4103: The usage string in the mapped object (the usage
property created in the .map callback for each action) is shell-specific by
embedding single-quoted JSON; update it to a shell-neutral example such as
showing file-based input or a documented cross-platform form. Replace the inline
'{"key":"value"}' example with something like --input-json-file
<path/to/input.json> or else show the JSON using an escaped form that you
document as platform-dependent; update the usage property in the map (the code
that builds name/usage for each action) so list_ade_actions returns a
shell-neutral, copy/paste-safe example.
---
Outside diff comments:
In `@apps/desktop/src/main/main.ts`:
- Around line 3369-3407: buildAdeActionRuntimeForAutomations is not including
linearCredentialService so getAdeActionDomainServices (which reads
runtime.linearCredentialService for the linear_credentials domain) gets
undefined; fix by adding linearCredentialService to the returned object in
buildAdeActionRuntimeForAutomations so the runtime includes
runtime.linearCredentialService (i.e., include a property named
linearCredentialService alongside the other services in the returned AdeRuntime
object) to ensure ade-action automation steps can access linear_credentials.
---
Duplicate comments:
In `@apps/ade-cli/src/adeRpcServer.ts`:
- Around line 5413-5419: When baseBranch is omitted, prSvc.draftDescription is
currently asked to infer the diff itself which can mismatch the stable base
resolution used later by createFromLane; instead resolve the exact base branch
that will be used (using the same resolver/helper or logic createFromLane uses)
into a variable (e.g., resolvedBaseBranch) and pass that resolvedBaseBranch into
prSvc.draftDescription({ laneId, ...(resolvedBaseBranch ? { baseBranch:
resolvedBaseBranch } : {})}) so the drafted title/body match the actual PR base;
update references around baseBranch, prSvc.draftDescription, createFromLane,
laneId, title and body accordingly.
In `@apps/ade-cli/src/bootstrap.ts`:
- Around line 526-582: The headless AdeRuntime object is missing the newly
required action services (keybindingsService, onboardingService,
feedbackReporterService, usageTrackingService, budgetCapService,
autoUpdateService) which causes getAdeActionDomainServices(runtime) to reject
those domains; add those properties to the runtime object and wire them to the
appropriate headless implementations (e.g.,
headlessLinearServices.keybindingsService,
headlessLinearServices.onboardingService, etc.) or provide lightweight/no-op
fallbacks from the same service factory, and ensure any added services are also
disposed in the dispose() swallow block (call their dispose/disposeAll if
available).
In `@apps/ade-cli/src/cli.ts`:
- Around line 1860-1887: VALUE_CARRIER_FLAGS is missing the short flag "-c",
which breaks buildShellPlan and buildTestsPlan because they call readValue(args,
["--command", "-c"]); update the ReadonlySet VALUE_CARRIER_FLAGS to include the
string "-c" alongside the other short-form flags (e.g., "-b","-m","-q","-t") so
that readValue can forward "-c" values correctly; after adding "-c", run the
related shell/tests paths and consider adding a TODO to derive this set from the
parser metadata to avoid future drift.
- Around line 2009-2017: The loop builds projectRoot/workspaceRoot from segments
but on Windows a drive root becomes "C:" (drive-relative) which causes
path.resolve(projectRoot) to resolve relative to the current directory on that
drive; detect Windows drive-only tokens (e.g., projectRoot matching
/^[A-Za-z]:$/) and append the path separator before using
projectRoot/workspaceRoot (or use path.join(projectRoot, path.sep)) so that
projectRoot becomes "C:\\" (true drive root) before calling fs.existsSync and
path.resolve; apply the same normalization for workspaceRoot and any checks that
use projectRoot.
In `@apps/desktop/src/main/main.ts`:
- Around line 3176-3249: The RPC runtime currently captures a null snapshot of
autoUpdateService; instead ensure rpcRuntime uses the real service instance by
assigning rpcRuntime.autoUpdateService = autoUpdateService after the updater is
initialized (or define a live getter on rpcRuntime that returns the current
autoUpdateService variable). Locate the rpcRuntime object and the
autoUpdateService variable and either (a) remove autoUpdateService from the
static literal and set rpcRuntime.autoUpdateService = autoUpdateService once the
updater is created, or (b) use Object.defineProperty(rpcRuntime,
"autoUpdateService", { get: () => autoUpdateService }) so socket RPC handlers
always see the live updater instance.
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Around line 67-83: The allowlist contains usage.poll without CTO gating, so
update ADE_ACTION_CTO_ONLY to include "poll" under the usage key to match the
privileged actions; specifically, add "poll" alongside "start", "stop", and
"forceRefresh" in ADE_ACTION_CTO_ONLY (so run_ade_action will properly enforce
CTO-only callers for poll(), which shares the collector with
start()/forceRefresh()); ensure ADE_ACTION_ALLOWLIST.usage still contains "poll"
if needed but the CTO-only gate must be added in ADE_ACTION_CTO_ONLY to close
the privilege gap.
- Around line 480-492: The layout.set handler currently treats any
non-plain-object as {} and overwrites saved layouts; change set (in the layout
action) to explicitly validate args.layout: allow only null (to clear) or a
plain object (non-array, typeof "object"); if args.layout is missing or is any
other type (primitive or array) throw an Error (message: "Invalid 'layout' —
pass an explicit null to clear or a plain object") instead of falling through to
{}. After validation, call clampDockLayout on the plain object and persist via
runtime.db.setJson(`dock_layout:${layoutId}`, layout) as before; keep
requireNonEmptyString(layoutId) and the existing error text for missing layout
key but tighten the type check.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 10918-10930: The ADE_CLI_AGENT_GUIDANCE entry is currently gated
by shouldInjectLaneDirective so non-Claude providers lose guidance after lane
persistence; change the condition that adds ADE_CLI_AGENT_GUIDANCE in the
composeLaunchDirectives array so it does not depend on shouldInjectLaneDirective
but instead checks that providerHasPersistentGuidance is false and the provider
is not Claude (use managed.session.provider or existing
buildClaudeInteractionModeDirective/interactionMode to detect Claude); i.e., add
ADE_CLI_AGENT_GUIDANCE when !providerHasPersistentGuidance &&
managed.session.provider !== 'claude' (or equivalent), leaving the lane
directive inclusion unchanged.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 732-755: getRowByLocator currently resolves bare numeric locators
via getRowByNumber which searches the whole project and can return the wrong PR
if multiple repos in the project have the same PR number; change the logic so
that when parsePrLocator returns only a number (no owner/repo) you first check
for ambiguity by counting matching rows for that projectId and number (use the
same query shape as getRowByNumber), and if more than one match exists either
scope the lookup to the active repo (call getRowForRepoPr(activeOwner,
activeRepo, parsed.number)) or return null/fail to avoid ambiguous resolution;
update getRowByLocator to use parsePrLocator, getRowForRepoPr, and projectId
accordingly and ensure ambiguous cases are rejected instead of returning the
most-recent row.
- Around line 1157-1210: The upsertRow() currently uses a fallback
getRowForRepoPr() and will unconditionally reassign lane ownership (deleting
pr_group_members and nulling integration_proposals) which can steal an active PR
from another lane; fix by splitting the lookup into two variables (e.g. const
laneRow = getRowForLane(...); const repoRow = getRowForRepoPr(...); const
existing = laneRow ?? repoRow) and if existing came from the repoRow fallback
(laneRow is null) and existing.lane_id !== summary.laneId then do not perform
the destructive handoff—either return existing.id unchanged or throw/return an
error—so lane transfers only happen via an explicit transfer flow instead of
silently rewriting lane_id in upsertRow().
---
Nitpick comments:
In `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Around line 3270-3283: Add a second test invocation of callTool(handler,
"create_pr_from_lane", { laneId: "lane-1" }) that omits baseBranch and assert
the result and service calls mirror the optional-arg contract: verify
drafted?.isError is undefined, that fixture.runtime.prService.draftDescription
was called for the lane without a baseBranch (e.g., calledWith({ laneId:
"lane-1" }) or with baseBranch undefined), and that
fixture.runtime.prService.createFromLane was lastCalledWith an object containing
laneId "lane-1" and either an inferred baseBranch or baseBranch undefined (match
the server behavior you expect) so the test covers the baseBranch-omitted path
for create_pr_from_lane.
In `@apps/ade-cli/src/cli.test.ts`:
- Around line 387-401: This test creates a real temp directory via rawRoot =
fs.mkdtempSync(...) but never removes it; wrap the test body in a try/finally
(or add an afterEach for this spec) and call fs.rmSync(rawRoot, { recursive:
true, force: true }) in the finally/afterEach to ensure the temp directory is
removed; reference the rawRoot variable created at the top of the test (and you
can use root = fs.realpathSync.native(rawRoot) as before) so cleanup removes the
actual on-disk directory created by mkdtempSync.
In `@apps/desktop/src/main/services/orchestrator/baseOrchestratorAdapter.ts`:
- Around line 798-799: The stored preview currently slices overridePrompt (which
includes ADE_CLI_AGENT_GUIDANCE) causing every preview to show the same
guidance; change startupCommandPreview to be derived from startupCommandOverride
(e.g., use startupCommandOverride.slice(0, 320)) so the preview reflects the
actual override, while keeping promptLength calculated from overridePrompt (to
include the full combined length); alternatively, if you prefer promptLength to
exclude guidance, compute promptLength from startupCommandOverride and keep
startupCommandPreview from overridePrompt—update the assignments around
promptLength, startupCommandPreview, overridePrompt, and startupCommandOverride
to reflect the chosen approach.
🪄 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: 11375018-1081-4863-b5eb-cfe9f1012b34
⛔ Files ignored due to path filters (1)
.claude/scheduled_tasks.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/cto/workerAdapterRuntimeService.test.tsapps/desktop/src/main/services/cto/workerAdapterRuntimeService.tsapps/desktop/src/main/services/orchestrator/baseOrchestratorAdapter.tsapps/desktop/src/main/services/orchestrator/coordinatorAgent.tsapps/desktop/src/main/services/orchestrator/orchestratorService.tsapps/desktop/src/main/services/orchestrator/promptInspector.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/types/prs.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/services/orchestrator/coordinatorAgent.ts
| .map((action) => ({ | ||
| domain: entry, | ||
| action, | ||
| name: `${entry}.${action}`, | ||
| usage: `ade actions run ${entry}.${action} --input-json '{"key":"value"}' (or --scalar value / --args-list-json '[...]' for scalar or positional service methods)`, | ||
| })); |
There was a problem hiding this comment.
Use shell-neutral usage examples here.
The new usage string hardcodes single-quoted JSON, which is copy/pasteable in POSIX shells but not in common Windows shells. Since list_ade_actions returns this directly to users, the example is now misleading on a supported platform.
💡 Suggested change
- usage: `ade actions run ${entry}.${action} --input-json '{"key":"value"}' (or --scalar value / --args-list-json '[...]' for scalar or positional service methods)`,
+ usage: `ade actions run ${entry}.${action} --input-json <json> (or --scalar <value> / --args-list-json <json-array> for scalar or positional service methods)`,📝 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.
| .map((action) => ({ | |
| domain: entry, | |
| action, | |
| name: `${entry}.${action}`, | |
| usage: `ade actions run ${entry}.${action} --input-json '{"key":"value"}' (or --scalar value / --args-list-json '[...]' for scalar or positional service methods)`, | |
| })); | |
| .map((action) => ({ | |
| domain: entry, | |
| action, | |
| name: `${entry}.${action}`, | |
| usage: `ade actions run ${entry}.${action} --input-json <json> (or --scalar <value> / --args-list-json <json-array> for scalar or positional service methods)`, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ade-cli/src/adeRpcServer.ts` around lines 4098 - 4103, The usage string
in the mapped object (the usage property created in the .map callback for each
action) is shell-specific by embedding single-quoted JSON; update it to a
shell-neutral example such as showing file-based input or a documented
cross-platform form. Replace the inline '{"key":"value"}' example with something
like --input-json-file <path/to/input.json> or else show the JSON using an
escaped form that you document as platform-dependent; update the usage property
in the map (the code that builds name/usage for each action) so list_ade_actions
returns a shell-neutral, copy/paste-safe example.
- orchestratorService: drop redundant ADE_CLI_AGENT_GUIDANCE prepend in augmentedPrompt; buildFullPrompt already injects it into the system prompt for mission-tooling workers, so the double-injection wasted context. - adeActions registry.graph_state: ignore caller-supplied projectId and always key by runtime.projectId — closes the cross-project read/write escalation. - adeActions registry.layout/tiling_tree/graph_state set: reject non-plain- object payloads in addition to missing keys (allow explicit null to clear). - adeActions registry CTO-only: gate usage.poll alongside start/stop/forceRefresh. - prService.upsertRow: no longer adopts rows matching by repo/PR number across lanes unless caller passes allowRepoPrAdoption. Only duplicate-PR recovery in createFromLane opts in; linkToLane + refresh stay lane-scoped. - prService.getRowByNumber: accepts optional repo owner/name; throws on cross-repo ambiguity so bare "#123" locators surface conflicts instead of silently picking the most-recently-updated row. - main.ts: autoUpdateService is initialized before rpcRuntime construction so the embedded socket RPC captures a live instance instead of null. - cli.ts VALUE_CARRIER_FLAGS: prune boolean-only flags (--delete-source, --delete-source-lane, --include-ignored, --unresolved). - agentChatService: decouple ADE_CLI_AGENT_GUIDANCE from shouldInjectLaneDirective. Providers without persistent system-prompt guidance (Codex/OpenCode/Cursor) now get the block on every launch, even resumed sessions. - providerOrchestratorAdapter.test: accept the now-always-augmented worker prompt via expect.stringContaining("diagnose the failing check") instead of an exact-equality assertion. Addresses 3134403033, 3134403054, 3134403060, 3134403076, 3134443870, 3134443879, 3134503369, 3134503423, 3134504179, 3134504183, 3134504190, 3134504196. Fixes the providerOrchestratorAdapter test that failed on CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
| const segments = resolved.split(path.sep); | ||
| for (let index = segments.length - 2; index >= 0; index -= 1) { | ||
| if (segments[index] !== ".ade" || segments[index + 1] !== "worktrees") continue; | ||
| const projectRoot = segments.slice(0, index).join(path.sep) || path.sep; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new worktree-root discovery reconstructs absolute paths by splitting on path.sep and joining slices back together. On Windows, a worktree under a drive-root project like C:\.ade\worktrees\lane\... produces segments[0] === "C:", so segments.slice(0, index).join(path.sep) becomes C: instead of C:\. ```ts
// apps/ade-cli/src/cli.ts
const segments = resolved.split(path.sep);
for (let index = segments.length - 2; index >= 0; index -= 1) {
if (segments[index] !== ".ade" || segments[index + 1] !== "worktrees") continue;
const projectRoot = segments.slice(0, index).join(path.sep) || path.sep;
That makes the `fs.existsSync(path.join(projectRoot, ".ade"))` check look at `C:.ade`, so managed-worktree detection fails and `findProjectRoots()` falls back to the worktree-local `.ade` directory instead of the parent project. I verified that `resolveAdeLayout(projectRoot)` keys the socket/db paths off this value, so CLI calls from that worktree will target the wrong ADE state. Fix by preserving the parsed filesystem root when rebuilding `projectRoot`/`workspaceRoot` rather than joining raw segments directly.
| pr: toService(runtime.prService), | ||
| tests: toService(runtime.testService), | ||
| chat: toService(runtime.agentChatService), | ||
| keybindings: toService(runtime.keybindingsService), |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
These new registry entries make domains like keybindings and onboarding part of the CLI action surface, but the actual headless runtime created in apps/ade-cli/src/bootstrap.ts still never instantiates keybindingsService, onboardingService, usageTrackingService, budgetCapService, feedbackReporterService, or autoUpdateService — it only added optional type slots for them. In practice that means the PR's "service expansion" is incomplete: list_ade_actions on the real CLI silently omits these domains and any direct run_ade_action call fails with Domain '...' is unavailable in this runtime. Wire the corresponding services in createAdeRuntime() or keep these domains out of the shared CLI registry until headless implementations exist.
// apps/desktop/src/main/services/adeActions/registry.ts
keybindings: toService(runtime.keybindingsService),
onboarding: toService(runtime.onboardingService),
automation_planner: toService(runtime.automationPlannerService),
...
usage: toService(runtime.usageTrackingService),
budget: toService(runtime.budgetCapService),
| // prepended even on resumed sessions where `shouldInjectLaneDirective` is | ||
| // false (review 3134504183 / 3134403060). | ||
| const providerHasPersistentGuidance = managed.session.provider === "claude"; | ||
| const shouldInjectGuidance = !providerHasPersistentGuidance; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
// apps/desktop/src/main/services/chat/agentChatService.ts
const providerHasPersistentGuidance = managed.session.provider === "claude";
const shouldInjectGuidance = !providerHasPersistentGuidance;
...
shouldInjectGuidance ? ADE_CLI_AGENT_GUIDANCE : null,This changed the guidance gate from a first-launch/session-state check into an unconditional per-turn check for every non-Claude provider. composeLaunchDirectives() is used for normal OpenCode/Codex/Cursor turns, so every follow-up message now gets the full ## ADE CLI block again; that directly contradicts the existing regression expectation in agentChatService.test.ts that the second OpenCode turn omits the guidance. It also creates an immediate double-injection path for worker sessions, because createWorkerAdapterRuntimeService.run() already prefixes runSessionTurn() input with the same guidance before handing control to agentChatService. Track whether ADE CLI guidance has already been delivered for the session/continuation and inject it once, rather than on every non-Claude turn.
| if (sub === "body" || sub === "update-body") return { kind: "execute", label: "PR update body", steps: [actionCallStep("result", "pr_update_body", withPr({ prId: prId ?? firstPositional(args), body: readValue(args, ["--body"]) ?? "" }))] }; | ||
| if (sub === "link") return { kind: "execute", label: "PR link", steps: [actionStep("result", "pr", "linkToLane", collectGenericObjectArgs(args, { laneId: readLaneId(args) ?? firstPositional(args), url: readValue(args, ["--url"]) }))] }; | ||
| if (sub === "link") { | ||
| const laneId = readLaneId(args) ?? firstPositional(args); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
buildPrPlan() now advertises support for linking by PR number, but the new link branch still consumes the first positional before stripping the PR locator flags. For ade prs link --number 123 lane-1, firstPositional(args) sees 123 (the value for --number) and assigns it to laneId, then readValue(... --number ...) reads the remaining positional lane-1 back as prUrlOrNumber. The call then reaches pr.linkToLane with swapped arguments and fails on the lane lookup instead of linking the PR. Read the explicit PR locator flags before falling back to a positional PR argument so --number/--pr-number work regardless of argument order.
// apps/ade-cli/src/cli.ts
const laneId = readLaneId(args) ?? firstPositional(args);
const prUrlOrNumber =
readValue(args, ["--url", "--pr-url", "--number", "--pr-number"])
?? firstPositional(args);| const laneId = readLaneId(args) ?? firstPositional(args); | |
| const explicitPrUrlOrNumber = | |
| readValue(args, ["--url", "--pr-url", "--number", "--pr-number"]); | |
| const laneId = readLaneId(args) ?? firstPositional(args); | |
| const prUrlOrNumber = explicitPrUrlOrNumber ?? firstPositional(args); |
| providersSection, | ||
| "", | ||
| "ADE CLI: Terminal-capable workers can use the bundled `ade` command for internal ADE actions. Instruct them to run `ade doctor` for readiness, `ade actions list --text` for discovery, typed commands such as `ade lanes list --text` or `ade prs checks <pr> --text` first, and `ade actions run ...` as the escape hatch. Use `--json` for structured output and `--text` for readable output.", | ||
| ADE_CLI_AGENT_GUIDANCE, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
// apps/desktop/src/main/services/orchestrator/promptInspector.ts
text: [
providersSection,
"",
ADE_CLI_AGENT_GUIDANCE,
].join("\n"),The inspector now claims the coordinator receives ADE_CLI_AGENT_GUIDANCE, but the actual coordinator system prompt built in coordinatorAgent.buildSystemPrompt() no longer contains any equivalent ## ADE CLI section after this diff removed the old inline block. That makes the prompt inspector inaccurate again: operators inspecting a run will see ADE CLI guidance in the synthetic prompt even though the live coordinator never saw it, which is exactly the kind of prompt/runtime divergence this tool is supposed to prevent. Keep the inspector and the real coordinator prompt in lockstep by either restoring the same guidance in coordinatorAgent or removing it from the inspector layer.
| // creation because a PR already exists for the head branch) is the only | ||
| // legitimate use of the repo/PR-number fallback; it opts in via | ||
| // `allowRepoPrAdoption: true`. | ||
| const existing = options?.allowRepoPrAdoption |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new upsertRow lookup only checks lane_id unless allowRepoPrAdoption is set, so linkToLane() will insert a second pull_requests row when the same GitHub PR is already linked to another lane. ts // apps/desktop/src/main/services/prs/prService.ts const existing = options?.allowRepoPrAdoption ? getRowForLane(summary.laneId) ?? getRowForRepoPr(summary.repoOwner, summary.repoName, summary.githubPrNumber) : getRowForLane(summary.laneId); The table schema has no uniqueness constraint on (project_id, repo_owner, repo_name, github_pr_number), but downstream code still assumes repo/PR uniqueness: getRowForRepoPr(... limit 1) picks one row arbitrarily, and getGithubSnapshot() collapses rows into a single Map entry by repoPrKey, so after a relink the older lane can win and the newly linked lane disappears from snapshot/group metadata. Reject cross-lane relinks explicitly or perform a deliberate migration flow; do not silently insert duplicate rows for the same external PR.
| return getRowByNumber(parsed.number); | ||
| }; | ||
|
|
||
| const getRow = (prIdOrLocator: string): PullRequestRow | null => |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This change makes every internal prId lookup accept #123/URL locators, but the snapshot-backed methods still persist snapshots under the raw argument instead of the resolved row ID. ts // apps/desktop/src/main/services/prs/prService.ts const getRow = (prIdOrLocator: string): PullRequestRow | null => getRowById(prIdOrLocator) ?? getRowByLocator(prIdOrLocator); getStatus, getChecks, getDetail, getFiles, and refreshSnapshotData all call upsertSnapshotRow({ prId, ... }), and pull_request_snapshots.pr_id is a foreign key to pull_requests(id) in @apps/desktop/src/main/services/state/kvDb.ts. So an IPC/sync call like prs.getStatus({ prId: "#123" }) now resolves the PR successfully and then writes "#123" as the snapshot key, which fails foreign-key validation (or at best creates an unusable orphan if FK enforcement is off). Normalize these code paths to the canonical row.id before persisting snapshots, or keep locator parsing scoped to APIs that only read by locator.
1 participant
ADE CLI RPC/bootstrap improvements, shared adeCliGuidance for agents, orchestrator/chat/CTO wiring, and safer integration lane ownership for cleanup (scratch lanes vs merge-into adoption).
Made with Cursor
Summary by CodeRabbit
Release Notes
New Features
Improvements
Greptile Summary
This PR consolidates ADE CLI guidance into a single shared constant (
ADE_CLI_AGENT_GUIDANCE), fixes several PR duplicate-recovery bugs (cascade-delete scoping,findExistingPrForBranch,#123locator parsing), expands the action allowlist with ~15 new domains and role-based access enforcement, and improves worktree-aware project root discovery in the CLI.All previously flagged issues appear resolved:
startupCommandPreviewnow logs the augmented prompt, duplicate guidance inpromptInspector.tsis removed, the|| Boolean(candidate?.draft)closed-draft matching is gone,--textis absent fromVALUE_CARRIER_FLAGS, andgraph_stateis correctly scoped toruntime.projectId.Confidence Score: 5/5
Safe to merge; all P0/P1 concerns from prior reviews are addressed and the two new findings are minor style inconsistencies.
Every previously flagged P0/P1 issue has a clear fix in this diff. The two new findings are P2: a null-vs-empty-object inconsistency in
layout.setand a hardcoded provider string gate. Neither causes data loss or incorrect behavior in the current provider set.apps/desktop/src/main/services/adeActions/registry.ts —
layout.set(null)inconsistency; apps/desktop/src/main/services/chat/agentChatService.ts — hardcoded"claude"provider check.Important Files Changed
hasHelpFlag/VALUE_CARRIER_FLAGSrouting, worktree-aware root discovery, and command aliases.--textno longer inVALUE_CARRIER_FLAGS, fixing the prior help-swallowing bug.graph_state,layout,tiling_tree,keybindings,github,budget, etc.), role-based access control helpers (isCtoOnlyAdeAction/callerHasRoleAtLeast), and inline domain-service builders.layout.set(null)stores{}instead of clearing, inconsistent withtiling_tree.setandgraph_state.set.getRowByLocator,repoPrKey,findExistingPrForBranchfor safer duplicate-PR recovery;upsertRowreturns the upserted ID; cascade deletes gated behindallowRepoPrAdoption;#123PR locator parsing;getIntegrationLaneOriginnull-safe guard;listAllnow accepts laneId filter.ADE_CLI_AGENT_GUIDANCEconstant; removes the duplicate "ADE TOOLING" push that previously resulted in two copies of the guidance in every worker prompt.ADE_CLI_AGENT_GUIDANCEinto startup-command override prompt;startupCommandPreviewnow logs the augmented prompt (not the bare override), fixing prior debug-visibility bug.ADE_CLI_AGENT_GUIDANCEmust NOT be prepended to the worker prompt file becausebuildFullPromptalready injects it via the system prompt for Claude workers.ADE_CLI_AGENT_GUIDANCE(multi-line) andADE_CLI_INLINE_GUIDANCE(single-line) to consolidate ADE CLI guidance strings across agents, inspector, and chat service.providerHasPersistentGuidance) using a hardcoded"claude"provider name comparison that may miss future providers.autoUpdateServicecreation to beforeinitContextForProjectRootso all RPC runtimes capture a live reference; exposes many additional services onrpcRuntimeto match the expandedADE_ACTION_ALLOWLIST.ADE_ACTION_DOMAIN_NAMESconst instead of hardcoded enum arrays; wiresisCtoOnlyAdeActioninto bothlist_ade_actions(filter) andrun_ade_action(enforcement) handlers;create_pr_from_lanetitleno longer required.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[ade CLI invocation] --> B{hasHelpFlag?} B -- yes --> C[Return HELP_BY_COMMAND text] B -- no --> D{Primary command} D --> E[prs create / link] D --> F[actions run] D --> G[git / lanes / files / ...] E --> H[prService.createFromLane] H --> I{GitHub API create PR} I -- success --> J[upsertRow - returns prId] I -- duplicate error --> K[findExistingPrForBranch] K --> L[Adopt existing PR row allowRepoPrAdoption:true] L --> J J --> M[markHotRefresh + refreshOne] F --> N{isCtoOnlyAdeAction?} N -- yes, caller not CTO --> O[Throw permission error] N -- no / caller is CTO --> P[Invoke domain service method] subgraph ADE_CLI_GUIDANCE [Shared guidance injection] Q[ADE_CLI_AGENT_GUIDANCE constant] Q --> R[baseOrchestratorAdapter - startup override prompt] Q --> S[promptInspector - worker base guidance] Q --> T[agentChatService - non-Claude providers only] Q --> U[ctoStateService - CTO doctrine] endComments Outside Diff (5)
apps/desktop/src/main/services/orchestrator/promptInspector.ts, line 178-225 (link)ADE_CLI_AGENT_GUIDANCEblock in every worker promptbuildWorkerBaseGuidanceunconditionally callssections.push(...)withADE_CLI_AGENT_GUIDANCEat two separate points (lines 178 and 213). Both execute for every worker, so the resulting prompt contains the## ADE CLIheading and its two-line body verbatim twice.Before this PR the two slots held distinct strings:
"ADE CLI: In terminal-capable sessions, use the bundled \ade` command…"`"ADE TOOLING: Use ADE's action surface or the \ade` CLI for team collaboration commands when available."`After the consolidation both slots emit the same constant. The worker identity / key-actions block in the second
sections.pushis still unique and correct, but it is now prefixed with a redundant## ADE CLIsection that will appear a second time in the prompt.Prompt To Fix With AI
apps/desktop/src/main/services/prs/prService.ts, line 2541-2559 (link)The condition used to trigger
findExistingPrForBranchis:"validation failed"is a broad phrase that GitHub also returns for unrelated schema violations (e.g. invalid base branch, missing required field). If such an error is misidentified as a duplicate-PR error, the code will silently look up and adopt an existing PR for the branch — potentially mapping the wrong PR to the lane. A narrower pattern such as/pull request already exists/i(or checking the GitHub422status code) would reduce the false-positive surface.Prompt To Fix With AI
apps/desktop/src/main/services/prs/prService.ts, line 1308-1330 (link)findExistingPrForBranchtyped asPromise<any | null>The function and its
fetchAllPages<any>call useany, which drops type safety on the returned GitHub PR object. The rest ofprService.tsaccesses.state,.draft,.number, etc. from the result with runtimeasString/Numberguards — that pattern works — but the signature makes it impossible for callers to know what shape they're consuming without reading the implementation.Prompt To Fix With AI
apps/desktop/src/main/services/orchestrator/promptInspector.ts, line 178-225 (link)ADE_CLI_AGENT_GUIDANCEinbuildWorkerBaseGuidanceADE_CLI_AGENT_GUIDANCEis pushed intosectionsat two separate points in this function (lines 178 and 213). Both calls execute unconditionally for every worker, so the inspector output contains the## ADE CLIheading and body verbatim twice. The inspector therefore shows a longer prompt than whatbuildFullPromptinbaseOrchestratorAdapter.tsactually sends to workers (where the guidance only appears once), making the inspector inaccurate for debugging.The second push (lines 213–225) is what contains the unique "worker identity" / key-actions text;
ADE_CLI_AGENT_GUIDANCEshould not prefix that block as well.Prompt To Fix With AI
apps/desktop/src/main/services/prs/prService.ts, line 1154-1215 (link)upsertRownow falls back togetRowForRepoPrwhengetRowForLanefinds nothing, and when it does match a PR that belongs to a different lane it immediately deletes that PR'spr_group_membersrows and nullifiesintegration_proposalsreferences. This path fires from bothcreateFromLane(duplicate-recovery) andlinkToLane.The
linkToLanecase is the risky one: if PR #X was already in a merge queue under lane A and a caller links it to lane B, the queue membership is silently destroyed with no notification or rollback. The caller has no signal that anything was cleared.Consider guarding the cascade deletes with a check that verifies neither
pr_group_membersnor activeintegration_proposalsexist before proceeding, or at minimum logging a warning when rows are removed.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (9): Last reviewed commit: "ship: iteration 4 (post-rebate) — post-m..." | Re-trigger Greptile