Fix Linear review bypass and headless usage tracking start

[cursor]

Bug and impact

1. Linear supervisor review bypass (critical)

Impact: Human review gates could be silently skipped. An agent, CLI, or automation could call resolveLinearRunAction({ action: "approve" }) while a workflow run was still in_progress on an earlier step. That pre-set reviewState: "approved". When advanceRun later reached the request_human_review step, it saw the pre-approved state and auto-completed the gate without any supervisor action.

Concrete trigger: Supervised worker workflow with a request_human_review step → agent calls resolveLinearRunAction with approve during worker delegation → worker finishes → review step is auto-approved.

2. Headless usage tracking never started (high)

Impact: ade serve / daemon bootstrap created usageTrackingService but never called start(), so usage snapshots stayed empty. Percent-based budget caps (weekly-percent, five-hour-percent) never blocked dispatch in headless mode because budgetCapService.checkBudget() returns { exceeded: false } when no usage data is available.

Root cause

1. resolveRunAction allowed approve/reject without checking run.status === "awaiting_human_review" or that the current step is request_human_review. The UI gated the Approve button correctly; service/tool/CLI paths did not.
2. Desktop main calls usageTrackingService.start() during project bind, but createAdeRuntime in apps/ade-cli/src/bootstrap.ts omitted the same call.

Fix

• Guard approve/reject in linearDispatcherService.resolveRunAction to require active supervisor review state.
• Call usageTrackingService.start() in ade-cli bootstrap after service construction.
• Add regression test: early approve is rejected before the review gate activates.

Validation

• npx vitest run src/main/services/cto/linearSync.test.ts -t "supervisor approval|early approve" (2 tests pass)
• npm --prefix apps/ade-cli run typecheck

Does not overlap open draft PRs #363–#372 (sync/CRR/orchestration/PTY fixes).

  

  Open in ADE  ·  cursor/critical-correctness-bugs-f4ea branch  ·  PR #374

Greptile Summary

This PR fixes two correctness bugs: a supervisor review bypass in the Linear dispatcher where approve/reject actions lacked run-status guards, and missing usageTrackingService.start() in the headless CLI bootstrap that caused percent-based budget caps to never trigger.

• Linear bypass fix: resolveRunAction now throws early if run.status !== "awaiting_human_review" or the current step is not request_human_review before mutating any state. A regression test covering both approve and reject was added.
• Headless usage tracking: usageTrackingService.start() is called after automationService.bindAdeActionRegistry() and before runtimeCreated = true, fitting cleanly into the existing try/finally cleanup path.
• Test rename: One existing adeRpcServer test was updated to reflect that read-only action calls no longer trigger operation audit records.

Confidence Score: 4/5

Both fixes are logically sound and well-scoped; the PR is safe to merge.

The dispatcher guard correctly prevents premature review state mutation, and the bootstrap change fits the existing try/finally cleanup lifecycle. The new regression test covers the core scenario, though its final assertion relies on implicit mock-driven state progression that could silently break if mocks change.

apps/desktop/src/main/services/cto/linearSync.test.ts — verify the second advanceRun reliably reaches awaiting_human_review under the mocked worker state

Important Files Changed

Filename	Overview
apps/desktop/src/main/services/cto/linearDispatcherService.ts	Core fix: approve/reject actions now guard on run.status and step type before mutating state; approve guard omits !reviewContext while reject includes it — asymmetry is intentional but optional chaining after the guard is redundant.
apps/ade-cli/src/bootstrap.ts	Adds usageTrackingService.start() just before runtimeCreated = true; fits within existing try/finally cleanup and dispose path — correct placement.
apps/desktop/src/main/services/cto/linearSync.test.ts	Adds it.each regression test for early approve/reject; correctly verifies the throw and unchanged run state; temp directory is not cleaned up but matches existing test patterns in the file.
apps/ade-cli/src/adeRpcServer.test.ts	Test renamed and assertions updated to match new behavior (read-only calls no longer emit operation audit records); change is consistent.
docs/features/linear-integration/dispatch-and-sync.md	Documentation updated to describe the new approve/reject guard semantics; accurate.
docs/features/remote-runtime/internal-architecture.md	Single sentence added describing when usageTrackingService starts in headless runtimes; accurate.

Sequence Diagram

sequenceDiagram
    participant Caller as Agent / CLI / UI
    participant Dispatcher as linearDispatcherService
    participant DB as KV Store

    Note over Caller,DB: Happy path
    Caller->>Dispatcher: advanceRun(runId)
    Dispatcher->>DB: "updateRun status=awaiting_human_review"
    Dispatcher-->>Caller: run awaiting_human_review
    Caller->>Dispatcher: resolveRunAction approve
    Dispatcher->>DB: check run.status OK
    Dispatcher->>DB: updateStep + updateRun approved
    Dispatcher-->>Caller: updated run

    Note over Caller,DB: Early approve fixed
    Caller->>Dispatcher: resolveRunAction approve [in_progress]
    Dispatcher-->>Caller: throws not awaiting supervisor review
    Note over DB: No mutation

Loading

Comments Outside Diff (1)

1. apps/desktop/src/main/services/cto/linearDispatcherService.ts, line 2442-2452 (link)

   After the guard asserts !reviewContext throws, the subsequent uses of reviewContext?.rejectAction with optional chaining are redundant — reviewContext is guaranteed non-null at this point. Using direct property access makes the intent clearer and avoids confusion about whether null is still possible here.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/desktop/src/main/services/cto/linearDispatcherService.ts
   Line: 2442-2452
   
   Comment:
   After the guard asserts `!reviewContext` throws, the subsequent uses of `reviewContext?.rejectAction` with optional chaining are redundant — `reviewContext` is guaranteed non-null at this point. Using direct property access makes the intent clearer and avoids confusion about whether `null` is still possible here.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

   

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/desktop/src/main/services/cto/linearDispatcherService.ts:2442-2452
After the guard asserts `!reviewContext` throws, the subsequent uses of `reviewContext?.rejectAction` with optional chaining are redundant — `reviewContext` is guaranteed non-null at this point. Using direct property access makes the intent clearer and avoids confusion about whether `null` is still possible here.

```suggestion
      updateRun(run.id, {
        reviewState: reviewContext.rejectAction === "loop_back" ? "changes_requested" : "rejected",
        latestReviewNote: note ?? "Rejected by reviewer.",
        status: reviewContext.rejectAction === "loop_back" ? "queued" : "in_progress",
        lastError: note ?? "Rejected by reviewer.",
        ...(reviewContext.rejectAction === "loop_back" ? {
          linkedSessionId: null,
          linkedWorkerRunId: null,
        } : {}),
      });
      if (reviewContext.rejectAction === "loop_back") {
```

### Issue 2 of 2
apps/desktop/src/main/services/cto/linearSync.test.ts:2321-2323
**Second `advanceRun` may not reach `awaiting_human_review` without a prior state change.** After the blocked action, the run is still in whatever state the first `advanceRun` left it (e.g., `awaiting_delegation` or `in_progress`). If `advanceRun` is not idempotent for that intermediate state — or if additional external signals are required — the second call could return the same blocked status rather than `awaiting_human_review`, making the final assertion flaky. The mock returns `listRuns: [{ status: "completed" }]` which should drive progression, but explicitly asserting the intermediate state before the second `advanceRun` call would make the progression assumptions visible.

_{Reviews (1): Last reviewed commit: "Fix ADE RPC action audit test" | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
ade	Ignored	Preview	May 28, 2026 7:17am

[arul28]

@copilot review but do not make fixes

[Copilot]

@copilot review but do not make fixes

Reviewed the latest commits in this PR and I don’t see any actionable correctness or security issues to fix. Per your request, I did not make code changes.

[cursor]

PR Review

Scope: 6 file(s), +90 / −18
Verdict: Looks good

This PR closes a Linear workflow correctness gap by requiring awaiting_human_review and an active request_human_review step before approve/reject mutate state, starts usageTrackingService in headless createAdeRuntime() after the ADE action registry is bound, and adds regression tests plus doc updates. The prior approve path could still call updateRun with reviewState: "approved" when the run was not on the review gate; that bypass is removed.

---

Notes

• Linear gate fix: approve previously guarded only updateStep while always running updateRun / appendEvent / workpad sync — early API or automation calls could skip supervisor review. Reject had no equivalent guards. New throws align with UI gating in OperationsSidebar.tsx (approve/reject only when reviewContext and awaiting_human_review).
• Headless usage: usageTrackingService.start() is idempotent (if (pollTimer) return), so the new bootstrap call is safe; placement after bindAdeActionRegistry matches the remote-runtime doc note about usage/budget actions.
• Test-only RPC change: adeRpcServer.test.ts now expects read-only list_lanes not to record operation metadata; no server behavior change in this diff.
• Error surfacing: Thrown messages from resolveRunAction propagate to chat tools (ctoOperatorTools catch) and the CTO UI (LinearSyncPanel actOnRun catch); no new unhandled IPC gap spotted for invalid approve/reject timing.

  

_{Sent by Cursor Automation: BUGBOT in Versic}

[greptile-apps]

Second advanceRun may not reach awaiting_human_review without a prior state change. After the blocked action, the run is still in whatever state the first advanceRun left it (e.g., awaiting_delegation or in_progress). If advanceRun is not idempotent for that intermediate state — or if additional external signals are required — the second call could return the same blocked status rather than awaiting_human_review, making the final assertion flaky. The mock returns listRuns: [{ status: "completed" }] which should drive progression, but explicitly asserting the intermediate state before the second advanceRun call would make the progression assumptions visible.

Prompt To Fix With AI

This is a comment left during a code review.
Path: apps/desktop/src/main/services/cto/linearSync.test.ts
Line: 2321-2323

Comment:
**Second `advanceRun` may not reach `awaiting_human_review` without a prior state change.** After the blocked action, the run is still in whatever state the first `advanceRun` left it (e.g., `awaiting_delegation` or `in_progress`). If `advanceRun` is not idempotent for that intermediate state — or if additional external signals are required — the second call could return the same blocked status rather than `awaiting_human_review`, making the final assertion flaky. The mock returns `listRuns: [{ status: "completed" }]` which should drive progression, but explicitly asserting the intermediate state before the second `advanceRun` call would make the progression assumptions visible.

How can I resolve this? If you propose a fix, please make it concise.