Skip to content

feat(desktop): add a Developer setting to stop starting a local gateway - #2917

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
ash663:feat/local-gateway-toggle
Aug 13, 2026
Merged

feat(desktop): add a Developer setting to stop starting a local gateway#2917
bolichen97 merged 1 commit into
kirodotdev:mainfrom
ash663:feat/local-gateway-toggle

Conversation

@ghost

@ghost ghost commented Aug 11, 2026

Copy link
Copy Markdown

Closes #2909

Problem

The desktop app starts a gateway of its own whenever nothing is answering on its port, even when it is configured to use a remote one and the user has no interest in running a backend locally. There is no setting for it: startGateway() probes the port and nothing else, and a configured remote host only changes how an already-occupied port is classified (resolveGatewayConflict treats the holder as non-local and reuses it). Whether you end up local or remote therefore depends on whether your connection happened to be up at launch, not on anything you chose.

Two things follow from that, and the second one is the sharper problem:

  • A machine that is only drawing the UI runs a Python backend plus a kiro-cli process per session, sub-agents and terminal shells included.
  • The tab name can be wrong. Tabs are named per port — customName, then remoteHosts[port].defaultName, then [:port] — so a named tab normally tells you which machine you are on. But that name comes from config, not from whatever is answering right now. When the remote is unreachable at launch and a local backend is started on the same port, the tab still shows the remote's name, and the app is quietly working against the local data home instead. applyTitle() has no way to know.

Why it matters

Running the gateway remotely is a supported, documented setup, and on a laptop the local backend is a cost paid for nothing. The silent fallback is worse than the waste: a user can be working against a different data home than the one their tab names, with no signal anywhere in the UI.

Scope, stated plainly: with the default on, that misnamed-tab fallback still happens for anyone who never finds this switch. This PR makes the harm opt-out rather than unavoidable — it does not solve it. The general fix is to make the tab reflect what actually answered on the port rather than what config says should have, which means probing the live responder and is a separate change; #2909 carries it.

Fix

Symptom — a client-only install gets an unwanted local gateway, and the tab keeps naming the remote.
Root cause — the decision to spawn is derived from a port probe, and no user intent is ever consulted.
Change — make the intent explicit and read it at the one place the decision is made.

  • runLocalGateway (default on, so nothing changes for existing installs) joins the app's electron-store config, with local-gateway.js holding the read/write rules: only an explicit false disables it, so a hand-edited config carrying "false" or 0 cannot silently stop the gateway starting.
  • A localGatewayAPI preload bridge plus two ipcMain.handle calls expose it to the renderer, following zoomAPI: the setting lives in the app's own config, which page JS cannot reach, and a missing bridge means the control is not rendered. That is what keeps it invisible in a plain browser and the PWA, where there is no local gateway to manage.
  • Both branches of startGateway() now funnel through one guard, so the choice cannot be honoured on one path and ignored on the other — including a takeover, where quitting the other channel's app frees the port on this machine but is not a request to run a gateway here.
  • With the setting off and nothing answering, the reason is reported as a fail-fast failure record rather than left to poll out the 30s timeout. describeGatewayFailure gives it wording that names both the port and the setting, and showLoadingThenConnect gives it its own dialog title instead of "gateway failed to start" — nothing failed, so sending the user hunting a crash log would be wrong. The dialog's existing Retry re-enters startGateway(), which is what makes "bring the connection up, then retry" work without a relaunch.

The setting takes effect on the next launch, stated under the switch, and is read once at launch into a session snapshot. That matters because startGateway() is also the recovery path for a gateway that died mid-session: re-reading the store there would let a flip made minutes ago refuse to replace a gateway the session is still using, stranding the user with no backend. The one thing that lifts the snapshot is the user explicitly asking for a gateway from the error dialog.

Recovery without a dashboard. The dialog cannot tell the user to open Settings, because Settings is served by a gateway — the thing that is not running. So the client-only variant carries a Start Local Gateway button that persists the setting, lifts the session snapshot and retries. Retry stays the primary action, since restoring the remote is the likelier fix.

Failure classification. classifyStartFailure() decides which story the dialog tells, and puts the client-only case ahead of the port-conflict probe. The launch log persists across launches, so an "address already in use" line left by an earlier run would otherwise offer to force-stop the holder of a port that is empty.

Placement. The toggle starts in Settings › Developer because that panel already exists and is where advanced switches live, so this needs no new Settings section and no decision about naming one. It is not the setting's permanent home — running everything remotely is a deployment choice, not a developer activity — but moving a toggle once there is a second setting to sit beside it is cheap, and it is called out in the component's own docstring so the placement is not mistaken for a claim.

Tests

  • electron/test/local-gateway.test.js (new) — the default-on rule, that only an explicit false disables it, that a non-boolean stored value ("false", 0, null, "") leaves the gateway enabled, and that writes are coerced to a real boolean. Plus classifyStartFailure: the client-only case outranks a stale port-in-use log line, a genuine conflict still wins when nothing is disabled, a bound port on another window's port is not our conflict, and spawn-failure vs timeout stay distinct.
  • electron/test/gateway-wait.test.js — the disabled failure message names the port and both ways out, does not send the user to Settings (unreachable in that state), avoids launch-failure wording, and wins over a stale error field on the same record.
  • src/test/useLocalGateway.test.ts (new) — reports unsupported and stays inert with no bridge, reads the stored value on mount, writes through, keeps the value the bridge returns rather than the one requested, and survives a rejecting bridge.
  • src/test/DeveloperPanel.test.tsx — the Gateway section is absent without the bridge and renders and writes through with it.

Each of these was mutation-checked: the behaviour it covers was broken in turn and the test confirmed to fail. That includes the new precedence rule — flipping the two branches in classifyStartFailure fails exactly one test.

Not covered: the glue inside startGateway() and the dialog markup itself, since main.js is not unit-testable. Both decisions they delegate to — isLocalGatewayEnabled and classifyStartFailure — are covered.

Manual verification

Local gates, all green — per CONTRIBUTING.md's documented commands:

  • cd website && npm run check (typecheck + eslint --max-warnings 1116 + vitest + electron): 994 files / 16,328 passed, 2 expected-fail, 3 skipped; 869/869 electron.
  • npm run i18n:check with I18N_BASE_REF set — every lane ok, including the DNT catalog check across all 11 translated catalogs.
  • npx jscpd . — 0 clones. ./scripts/scrub-lint.sh --no-history — passed. scripts/check_brand_name.py — passed.

No backend files are touched, so the Python gates do not apply.

Two pre-existing guards caught real defects during development, both fixed here: shell-contract.test.js flagged that local-gateway.js was missing from the electron-builder allowlist (it would have worked from source and been absent from the packaged DMG), and the settings-registry anti-stale guard required regenerating settingsRegistry.gen.ts — which also makes the toggle reachable from the command palette as developer.run-a-local-gateway.

The rendered surface is confirmed on macOS (see Screenshots): the branch's Electron shell against a pod serving this branch's frontend shows the Gateway section with the toggle in its default-on state.

Still unverified: the full off-path — turning the switch off, relaunching with the remote unreachable, confirming the new dialog, then restoring the connection and hitting Retry. That needs a packaged build against a real remote host, and the six lines of glue in startGateway() are the part no unit test covers.

Screenshots

Settings › Developer, captured from the branch's own Electron shell (npm start in website/electron) connected to an isolated pod serving this branch's frontend — so both halves of the change are the ones under review, not a released shell against a new bundle.

Settings > Developer showing the new Gateway section with the "Run a local gateway" toggle enabled

The section's presence is itself the evidence that the preload bridge resolved: useLocalGateway reports supported: false and renders nothing when window.localGatewayAPI is absent, which is why this surface does not exist in a browser or the PWA and cannot be captured from a loopback URL. DeveloperPanel.test.tsx asserts both halves of that.

@ghost
ghost self-requested a review August 11, 2026 23:41
@ghost
ghost self-requested a review as a code owner August 11, 2026 23:41
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 11, 2026
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Advisory design-level review of 09ca90ba4d16580da8375c2a3d1cf77aba42cad8 via the fork AI-review pipeline — updated in place on each push; does not block merge.

Design-Verdict: PASS

Sound, well-scoped intent switch at the single decision point; the design's own limits are honestly stated and deferred correctly — except one tracking slip.

Watch

[DESIGN-REVIEWED] 09ca90b

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 09ca90ba4d16580da8375c2a3d1cf77aba42cad8 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 09ca90b

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

Advisory UX-level review of 09ca90ba4d16580da8375c2a3d1cf77aba42cad8 via the fork AI-review pipeline — updated in place on each push; does not block merge.

UX-Verdict: CONCERNS

The recovery dialog's "Start Local Gateway" silently re-enables the setting permanently, and the palette deep-links browser users to a toggle that isn't rendered.

Watch

  • "Start Local Gateway" in the error dialog reads as a one-time action, but the handler runs setLocalGatewayEnabled(store, true) — a deliberately opted-out user who clicks it once to survive a remote outage is silently opted back in, and every future launch recreates the exact wrong-data-home harm this PR exists to prevent. Low frequency × high impact × persists across launches. Fix: say so in the button or message ("Start Local Gateway (turns the setting back on)"), or lift only the session snapshot.
  • settingsRegistry.gen.ts gains developer.run-a-local-gateway, but the registry is static while the toggle renders only when localGatewaySupported: a browser/PWA user searching "gateway" in the palette gets a result that navigates to Settings › Developer and highlights nothing. Every browser hit, moderate friction. Fix: mark the entry desktop-only or filter unsupported entries from the provider.
  • The client-only dialog keeps the launch-log <pre> and "Reveal Log" button (showGatewayErrorDialog renders logTail unconditionally) even though the copy insists nothing failed — a stale "address already in use" tail sends the user hunting the crash the message says doesn't exist. Fix: suppress log tail + Reveal Log when localGatewayOff.

Suggestions

  • When the user flips the toggle off with no remote host configured, the description's "the gateway you have configured" is false; add a one-line inline warning in DeveloperPanel for that state.

[UX-REVIEWED] 09ca90b

@ghost
ghost force-pushed the feat/local-gateway-toggle branch from 15b99c2 to b68cf54 Compare August 12, 2026 00:10
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 12, 2026
The desktop app decides whether to start its own gateway by probing the port,
not by asking what the user wants: a configured remote host only changes how an
ALREADY-occupied port is classified. So a client-only install gets a local
gateway whenever its connection is not up yet at launch — a Python backend plus
a kiro-cli process per session on a machine that is only drawing the UI — and
the tab keeps showing the remote's name, because that name comes from config
rather than from whatever is answering.

Add `runLocalGateway` (default on) to the app's electron-store config, exposed
as a Settings > Developer toggle over a new preload bridge. When it is off and
nothing is answering, startGateway reports a fail-fast reason instead of
spawning, so the existing error dialog names the port and the setting and its
Retry re-enters startGateway once the connection is up.

Both branches of startGateway funnel through one guard, so a takeover that frees
the port by quitting the other channel's app does not start a gateway here
either.
@ghost
ghost force-pushed the feat/local-gateway-toggle branch from b68cf54 to 09ca90b Compare August 12, 2026 00:16
@ghost

ghost commented Aug 12, 2026

Copy link
Copy Markdown
Author

Both findings on 15b99c2b were legitimate. Fixed in 09ca90ba.

main.js:624 — re-read of the setting during liveness recovery: FIXED.
Correct and the more serious of the two. startGateway() is the recovery path for a gateway that died mid-session, so re-reading the store there let a mid-session flip refuse to replace a gateway the session was still using — no backend, and the "next launch" contract the UI states broken in the worst direction. The value is now read once at launch into a session snapshot (let runLocalGateway = isLocalGatewayEnabled(store) at main.js:105) and the guard reads the snapshot. The IPC getter still reads the store, so the toggle renders the persisted value rather than the snapshot. One deliberate exception lifts it: the error dialog's new Start Local Gateway action, which is the user asking for a gateway right now.

main.js:2406portConflict shadowing err.failure.disabled: FIXED.
Also correct, and worse than "wrong title": the launch log persists across launches, so an address already in use line from an earlier run would offer to force-stop the holder of a port that is empty. Rather than reorder two inline booleans, the precedence is now a pure helper — classifyStartFailure() in local-gateway.js — returning client-only | port-conflict | failed | unreachable, with client-only ahead of the log probe. It carries four unit tests, including one that fails if the two branches are swapped, so the ordering is locked rather than asserted in a comment.

Thanks — the first one would have been a nasty field report.

@ghost

ghost commented Aug 12, 2026

Copy link
Copy Markdown
Author

Re: UX Review 🟡 CONCERNS — two points, two different dispositions.

1. Circular recovery path: FIXED in 09ca90ba.
This was the best finding on the PR and it was my error. The message told the user to open Settings › Developer, and Settings is served by the gateway — so the instruction was unreachable in precisely the state that printed it. The only real exits were restoring the remote or hand-editing JSON, and I shipped copy that implied otherwise.

Both halves are fixed:

  • The dialog's client-only variant now carries a Start Local Gateway button, which persists runLocalGateway = true, lifts the session snapshot so the retry actually spawns, and re-enters startGateway(). Retry stays the primary action, since a client-only user's likelier fix is restoring the remote — the new button is the escape hatch, not the default.
  • The failure message no longer mentions Settings at all. It now ends "…and retry, or start one here", pointing at the button that exists. A unit test asserts the string does not contain Settings, so the unreachable instruction cannot come back.

2. Palette advertises the toggle where it does not exist: REBUTTED (pre-existing, not introduced here).
Real inconsistency, but it is a property of the static registry rather than of this change. settingsRegistry.gen.ts is generated by scanning source, so it already lists settings that do not render in every client — computer-use.enable-computer-use and its four siblings (macOS-only, opt-in) and Zoom Level (the stepper is replaced by a shortcut hint when zoomAPI is absent) are all in there today. Teaching the registry about client capability is a registry-wide change with five-plus existing entries to migrate, and doing it as a rider on this PR would widen a Developer-tab toggle into a palette refactor. Worth filing separately; I have not filed it yet and will if you would rather it not sit unrecorded.

@ghost

ghost commented Aug 12, 2026

Copy link
Copy Markdown
Author

Re: Design Review 🟡 CONCERNS — ACCEPTED, and the PR body was overstating. Corrected in 09ca90ba.

You are right on both counts, and the second one is a fair hit on my description rather than on the code.

The framing was wrong. I led with the silent-fallback/misnamed-tab harm as the sharper problem, then shipped a change that leaves it intact in the default state. With runLocalGateway defaulting on — and it has to default on, or this stops being backward-compatible — every install that never visits the Developer tab still hits exactly the fallback I described. The body now says so plainly: this makes the harm opt-out rather than unavoidable, and does not solve it, with the general fix named as follow-up.

On the general fix: accepted-and-deferred, not dismissed. Making the UI reflect the actual responder rather than the configured name is the right answer, and it is a different change from this one. applyTitle() resolves customNameremoteHosts[port].defaultName[:port], all of it config, and none of it has any notion of what answered. Fixing it properly means introducing a live-responder signal and deciding what the tab should say when the probe is ambiguous — a tunnel presents as localhost, which is the reason isGatewayLocal() already has to consult config instead of the URL. That is a design decision about tab semantics, not a rider on a Settings toggle.

It is recorded on #2909 as the remaining half rather than left implicit, so it does not get lost when this merges.

On your suggestion specifically — flagging the tab when the configured remote name does not match what answered — that is the shape I would build, and it also happens to be the cheapest version, since it needs one probe at connect time rather than continuous monitoring. Noting it there.

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 09ca90ba4d16580da8375c2a3d1cf77aba42cad8 via the fork AI-review pipeline; updated in place on each push.

Review details

The single candidate concerns the use-react-query regex rule matching line 520 of useLocalGateway.ts. Falsifying it:

  • The rule is blocking: false — it can never block regardless.
  • Step 1 requires an observable wrong outcome (c). The candidate itself concedes the code is behaviorally correct: it drives an Electron IPC bridge (window.localGatewayAPI), not HTTP server state, so React Query's deduped-cache intent does not apply. There is no stale data, no crash, no wrong result — only a textual regex match.
  • A regex-fired lint warning with no behavioral defect falls squarely in the "lint warnings" category this pipeline is told to reject, and it fails to answer (c).

It dies under falsification.

No findings.

[OPUS-REVIEWED] 09ca90b

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 12, 2026
@bolichen97
bolichen97 merged commit 5d64560 into kirodotdev:main Aug 13, 2026
67 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 13, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #4259 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #4259: CONTINUE_DEVELOPMENT. The merged feature supplies the toggle and generic help text but not the prerequisite documentation this PR adds, so the docs gap is real and unclosed; the PR's own content defects (launch order, paragraph placement) still need a fix before merge. Files: website/electron/local-gateway.js.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@ash663
ash663 deleted the feat/local-gateway-toggle branch September 7, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting to turn off the local gateway when you only use a remote one

2 participants