feat(desktop): support runtime override fallback#467
Conversation
Greptile SummaryThis PR wires runtime-override support into the desktop spawn path: POSIX and Windows shims read
Confidence Score: 5/5Safe 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 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.
|
| 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
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 |
There was a problem hiding this 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.
| 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.| endlocal | ||
| exit /b 1 |
There was a problem hiding this 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.
| 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; |
There was a problem hiding this 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.
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.| if (this.terminated) return; | ||
| try { | ||
| await this.startOwnRuntime(); | ||
| } finally { | ||
| this.overrideRetryInFlight = false; | ||
| } |
There was a problem hiding this 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.
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.
Split from #440.
This PR wires runtime override support into the desktop spawn path, without scheduling background updates yet: