Skip to content

feat(desktop): support runtime override fallback#467

Open
cline-cloud[bot] wants to merge 1 commit into
cline/split-440-runtime-stage-codefrom
cline/split-440-runtime-spawn
Open

feat(desktop): support runtime override fallback#467
cline-cloud[bot] wants to merge 1 commit into
cline/split-440-runtime-stage-codefrom
cline/split-440-runtime-spawn

Conversation

@cline-cloud
Copy link
Copy Markdown

@cline-cloud cline-cloud Bot commented May 8, 2026

Split from #440.

This PR wires runtime override support into the desktop spawn path, without scheduling background updates yet:

  • packaged POSIX/Windows shims honor KANBAN_CLI_OVERRIDE
  • RuntimeChildManager forwards cliEntryOverride
  • RuntimeOrchestrator resolves an override per spawn
  • staged-runtime startup failure calls the rollback hook and retries the same launch with bundled runtime once
  • tests cover override wiring and fallback behavior

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 8, 2026

Greptile Summary

This PR wires runtime-override support into the desktop spawn path: POSIX and Windows shims read KANBAN_CLI_OVERRIDE, RuntimeChildManager forwards it from options, and RuntimeOrchestrator evaluates a per-spawn resolver and triggers a single bundled-CLI fallback retry when a staged spawn fails.

  • Shim changes (build/bin/kanban, kanban.cmd): fail-loud if the override path is missing, preventing silent fallback that would desync the host's rollback bookkeeping.
  • RuntimeChildManager: adds cliEntryOverride option, normalized via || undefined, and injects it as KANBAN_CLI_OVERRIDE in the child env only when defined.
  • RuntimeOrchestrator: adds resolveCliEntryOverride (re-evaluated every spawn) and onCliEntryOverrideFailed callback, with overrideRetryInFlight latch preventing infinite retry loops if the bundled CLI is also broken.

Confidence Score: 5/5

Safe to merge; the fallback path and latch logic are correct and well-covered by tests. Two previously-flagged concerns in kanban.cmd and runtime-orchestrator.ts remain open but are low-impact edge cases.

The orchestrator's override-capture, latch, and retry mechanics are sound: the overrideRetryInFlight flag reliably prevents infinite loops, currentSpawnOverridePath is captured at spawn time (not failure time) to avoid racing with concurrent staging, and the resolver is re-evaluated on every createManager() call so freshly-staged versions take effect on restart. The POSIX shim quotes all variable expansions correctly. The test suite covers the critical paths — success, one-shot retry, double failure, no-override, and resolver-throws.

packages/desktop/src/runtime-orchestrator.ts and packages/desktop/build/bin/kanban.cmd have open findings from the previous review round that have not yet been addressed.

Important Files Changed

Filename Overview
packages/desktop/src/runtime-orchestrator.ts Core fallback logic: per-spawn resolver, overrideRetryInFlight latch, currentSpawnOverridePath capture. The terminated early-return inside the fallback block exits before the finally, leaving overrideRetryInFlight permanently true on a shutdown race — previously flagged.
packages/desktop/src/runtime-child.ts Adds cliEntryOverride option, normalized via `
packages/desktop/build/bin/kanban POSIX shim correctly double-quotes $KANBAN_CLI_OVERRIDE at every expansion site and fails loudly on a missing override file.
packages/desktop/build/bin/kanban.cmd Windows shim: the echo line expands %KANBAN_CLI_OVERRIDE% unquoted (metacharacter injection risk) and the early-exit uses endlocal inconsistently — both previously flagged in review threads.
packages/desktop/test/runtime-orchestrator.test.ts Adds a comprehensive describe block covering override forwarding, null/missing resolver, fallback retry success, double-failure (no infinite loop), non-override failure, and resolver-throws cases. Static FakeChildManager fields are reset in beforeEach to avoid cross-test contamination.
packages/desktop/test/runtime-child-manager.test.ts Three focused tests: override forwarded in env, absent when omitted, and empty-string treated as no-override. All pass the right assertions on the spawn call's env object.

Sequence Diagram

sequenceDiagram
    participant Caller as connect() / restart()
    participant Orch as RuntimeOrchestrator
    participant Res as resolveCliEntryOverride()
    participant Mgr as RuntimeChildManager
    participant Shim as POSIX/Windows shim

    Caller->>Orch: startOwnRuntime()
    Orch->>Res: resolveCliEntryOverride()
    Res-->>Orch: "/staged/v1/cli.js" (currentSpawnOverridePath set)
    Orch->>Mgr: "new RuntimeChildManager({ cliEntryOverride })"
    Mgr->>Shim: "spawn with KANBAN_CLI_OVERRIDE=/staged/v1/cli.js"
    Shim-->>Mgr: exit (missing/broken)
    Mgr-->>Orch: start() throws

    Note over Orch: catch: failedOverride set, overrideRetryInFlight=true
    Orch->>Orch: onCliEntryOverrideFailed("...", "/staged/v1/cli.js")
    Note over Orch: host clears pointer

    Orch->>Orch: startOwnRuntime() [retry]
    Orch->>Res: resolveCliEntryOverride()
    Res-->>Orch: null (pointer cleared) → undefined
    Orch->>Mgr: "new RuntimeChildManager({ cliEntryOverride: undefined })"
    Mgr->>Shim: spawn without KANBAN_CLI_OVERRIDE (bundled CLI)
    Shim-->>Mgr: healthy
    Mgr-->>Orch: URL
    Note over Orch: finally: overrideRetryInFlight=false
    Orch-->>Caller: resolves
Loading

Reviews (2): Last reviewed commit: "feat(desktop): support runtime override ..." | Re-trigger Greptile

REM on missing file rather than silently falling back to bundled.
if defined KANBAN_CLI_OVERRIDE (
if not exist "%KANBAN_CLI_OVERRIDE%" (
echo error: KANBAN_CLI_OVERRIDE points to missing file: %KANBAN_CLI_OVERRIDE% >&2
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The echo line expands %KANBAN_CLI_OVERRIDE% without quoting. On Windows, if the value contains shell metacharacters (&, |, >, <), cmd.exe splits the line at those characters and executes the trailing token as a separate command. The POSIX shim double-quotes $KANBAN_CLI_OVERRIDE at every expansion site; the same caution applies here.

Suggested change
echo error: KANBAN_CLI_OVERRIDE points to missing file: %KANBAN_CLI_OVERRIDE% >&2
echo error: KANBAN_CLI_OVERRIDE points to missing file: "%KANBAN_CLI_OVERRIDE%" >&2
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/build/bin/kanban.cmd
Line: 18

Comment:
The `echo` line expands `%KANBAN_CLI_OVERRIDE%` without quoting. On Windows, if the value contains shell metacharacters (`&`, `|`, `>`, `<`), cmd.exe splits the line at those characters and executes the trailing token as a separate command. The POSIX shim double-quotes `$KANBAN_CLI_OVERRIDE` at every expansion site; the same caution applies here.

```suggestion
    echo error: KANBAN_CLI_OVERRIDE points to missing file: "%KANBAN_CLI_OVERRIDE%" >&2
```

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

Comment on lines +19 to +20
endlocal
exit /b 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The endlocal call here is inconsistent with every other early-exit path in this script. The parallel error paths (e.g., "Kanban CLI not found") do not call endlocal before exit /b 1, and neither does the POSIX shim have an equivalent. Removing it keeps all error exits uniform.

Suggested change
endlocal
exit /b 1
exit /b 1
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/build/bin/kanban.cmd
Line: 19-20

Comment:
The `endlocal` call here is inconsistent with every other early-exit path in this script. The parallel error paths (e.g., "Kanban CLI not found") do not call `endlocal` before `exit /b 1`, and neither does the POSIX shim have an equivalent. Removing it keeps all error exits uniform.

```suggestion
    exit /b 1
```

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

* capturing it at spawn time avoids racing with a concurrent
* background staging that may have moved the pointer to a *new*
* version (which we must not roll back). */
onCliEntryOverrideFailed?: (reason: string, cliEntry: string) => void;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Async callback silently discarded

onCliEntryOverrideFailed is typed as () => void, but TypeScript allows assigning an async function here. The retry call site does not await the return value, so any async work in the callback races with the immediately-following resolveCliEntryOverride() call. If the pointer is not yet cleared, the retry will pick up the same staged path again instead of falling back to the bundled CLI.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/src/runtime-orchestrator.ts
Line: 21

Comment:
**Async callback silently discarded**

`onCliEntryOverrideFailed` is typed as `() => void`, but TypeScript allows assigning an `async` function here. The retry call site does not `await` the return value, so any async work in the callback races with the immediately-following `resolveCliEntryOverride()` call. If the pointer is not yet cleared, the retry will pick up the same staged path again instead of falling back to the bundled CLI.

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

Comment on lines +393 to +398
if (this.terminated) return;
try {
await this.startOwnRuntime();
} finally {
this.overrideRetryInFlight = false;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 overrideRetryInFlight not reset on early terminated return

The if (this.terminated) return at line 393 exits before the try/finally block, so overrideRetryInFlight remains true if shutdown() races in at that exact point. In practice this is benign — a terminated orchestrator spawns nothing further — but moving the check inside the try would make the reset unconditional and easier to reason about.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/desktop/src/runtime-orchestrator.ts
Line: 393-398

Comment:
**`overrideRetryInFlight` not reset on early `terminated` return**

The `if (this.terminated) return` at line 393 exits before the `try/finally` block, so `overrideRetryInFlight` remains `true` if `shutdown()` races in at that exact point. In practice this is benign — a terminated orchestrator spawns nothing further — but moving the check inside the `try` would make the reset unconditional and easier to reason about.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant