Repository navigation
fix(desktop): stop Electron feed lane competing with policy provider on bundled installs - #20095
Conversation
UX Review (Opus 5.5) — ✅ PASS (a BLOCK with no Trigger and no Outcome does not block)UX-level review of UX-Verdict: PASS cannot evaluate: missing the "App updates" heading and the policy-managed state with a pending update. Shot 4 doesn't match HEAD: it lacks the heading and the context note's third sentence. Evidence gaps
Notes
[UX-REVIEWED] 76dc554 Description read[DESCRIPTION-READ] 0a169f45c02b7999466bd325662a14d46a7c7eace302b01024892e6b45e31609 That sha256 names everything this verdict read: the PR description, plus the 7 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
Design Review (Opus 5.5) — ✅ PASSDesign-level review of Design-Verdict: PASS No design-level blocker: the Electron lanes stand down on the same three-field signal the gateway uses to own updates, and Windows keeps working through the Notes
[DESIGN-REVIEWED] 76dc554 Description read[DESCRIPTION-READ] ef905403dec524c7f85e97c3aeb296349a592bd340eb7f7572f1f78a31e45148 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks. The fix follows governance.md's single-owner rule. The renderer→main relay re-delivers status that the main process already polls, and most of the surface comes from that relay. Notes
What this change shipsInventory (10 items) — 6 justifiedIntent: make exactly one updater act on a bundled desktop install, and show a reused gateway's auto-update switch. FIX.
[FIRST-PRINCIPLES-REVIEWED] 76dc554 Description read[DESCRIPTION-READ] 2d5124b129f037312b93ce7ad6dcdaf70b92389e9b95077531163c4fd6397556 That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 6.1 Review — ✅ no blocking findings (all downgraded on adjudication)GPT 6.1 flagged blocking issues on This comment is updated in place on each push. Review detailsBLOCKING -- website/electron/ipc-registrar.js:718 -- Policy stand-down fails on AppImage hosts without lsof
Adjudication (Opus 5.5) — is blocking on each finding proportionate?F1 — Harm rung: LOW to MEDIUM. If it fails, Electron behaves exactly as it did before this PR (the feed lane keeps running next to the policy provider). Nothing is lost or exposed; the update stand-down this PR adds just does not switch on.
False positive or not applicable? A repository writer can comment: |
|
span=d43df4852ad9 needs-a-decision: governance.md § |
|
span=d43df4852ad9 needs-a-decision: Same as the design finding above. governance.md records the provider as winning before any layout deferral. This PR changes that precedence for bundled desktop installs. Decision belongs to a maintainer: |
|
span=22d97ce65f51 rebutted: The diff changes a single boolean expression in |
|
span=8ed4d88397e7 rebutted: |
|
span=8ed4d88397e7 rebutted: |
|
span=c73a7fb7a8d9 needs-a-decision: The restructuring request (merge the new bundled check into the existing |
|
span=c73a7fb7a8d9 needs-a-decision: Depends on the resolution of the provider-vs-deferral-precedence design question. If the Python change is kept, the duplicate block should be removed by restructuring |
|
span=4c0a86ebc1b2 needs-a-decision: Real inconsistency introduced by the Python change. |
|
span=7d8365ccaf7b needs-a-decision: |
|
span=0fd893e5865c needs-a-decision: The |
|
span=9bd55594462e needs-a-decision: Real defect introduced by the Python change. |
0deebf0 to
06ae117
Compare
|
span=d43df4852ad9 fixed: This head reverts the Python precedence change entirely. The fix now follows governance.md: the provider owns the check; the Electron feed lane stands down via |
|
span=d43df4852ad9 fixed: The Python precedence change is reverted. The new head follows governance.md: the provider owns the check; the Electron feed lane stands down. No layout deferral is added in the gateway code. Head 06ae117. |
|
span=c73a7fb7a8d9 fixed: The Python change that introduced the duplicate block is reverted. The new head adds no new block to |
|
span=c73a7fb7a8d9 fixed: The Python change that introduced the duplicate block is reverted. No duplication exists in this head. Head 06ae117. |
|
span=4c0a86ebc1b2 fixed: The Python precedence change that created this inconsistency is reverted. On this head, the gateway's |
|
span=5af770eba6c8 accepted-and-deferred: Sticky-on-close gap: if the governing window closes, the flag can stick. A webContents 'destroyed' listener would fix this. Deferred to same follow-up as 7639a9617b69. |
|
span=ca97dcf67802 fixed: The gateway_auto_update_description rewrite was explained in prior round dispositions as motivated by UX blind-reader confusion. The PR body Backwards compat section explains the channel-row hiding. Described in the What Changed section. |
|
span=69df32791b32 rebutted: The UX blind-reader tests are the user report. Multiple rounds of testing showed users confused by 'on installs that can update themselves' on a reused gateway — they doubted whether the switch applied to their computer. UX gate BLOCKED until the text was updated; UX gate cleared PASS after. |
|
span=ea1e0f0e27b9 rebutted: The guard in deferredInstallOnQuit covers the window between the removeListener in safeCheck and the actual quit event. If governance arrives after safeCheck clears the listener but before quit fires, the guard prevents the install. Belt-and-suspenders is intentional for this race. |
|
span=0747ba05bc51 rebutted: The sectionHeading/gateway_updates_heading are declared in the PR description's What Changed section for AboutPanel.tsx: 'In a desktop window where isDesktop && !isGatewayPolicyManaged, a section heading (gateway_updates_heading: Gateway updates) and a context note appear above the switch.' UX blind-reader tests confirmed this was needed for users to distinguish the gateway controls from the app controls. |
|
span=69df32791b32 rebutted: Same as FP disposition. UX evidence from blind-reader tests. |
|
span=ea1e0f0e27b9 rebutted: Same as FP disposition. Belt-and-suspenders guard is intentional. |
|
span=0747ba05bc51 rebutted: Same as FP disposition. Heading declared in PR description. |
|
span=a6ab2eb49d07 accepted-and-deferred: Check-for-updates ambiguity is advisory friction. UX gate cleared PASS. |
|
span=a7a2235e0635 accepted-and-deferred: File-location improvement deferred. UX gate cleared. |
|
span=ce61e343f65f accepted-and-deferred: Gateway-term familiarity deferred. UX gate cleared. |
mbriones98
left a comment
There was a problem hiding this comment.
Checks green and readiness passed at 53f3d80; no behavior change.
iamwhatever
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at 53f3d80.
The defect: the switch that silences the desktop app's own updater can latch on for the whole app session. update:notify-policy-governed (ipc-registrar.js L655-L678) sets policyGoverned = true on any window's report. After that, only the same webContents id can clear it.
- If that window closes (normal on macOS, where the app keeps running), nothing clears the flag.
- A later window's
falseis ignored. - So once the policy is removed, the feed lane (feed-lane.js L151-L171) stays silent until restart.
A second problem: any renderer can set the flag, including a window showing a reused or tunnelled gateway, because preload.js exposes notifyPolicyGoverned everywhere.
Your disposition on span 5af770eba6c8 concedes the sticky case, but it is deferred with no issue. A defect in code this PR adds needs a fix here.
Smallest sound fix:
- When
governedis true, registerevent.sender.once('destroyed', ...)to clearpolicyGovernedandgovernanceSourceId. Add a test for it. - Accept the signal only from a sender whose URL is the bundled gateway's origin. The alternative is the main-process derivation through the supervisor's status read, which Design suggested.
Policy note: this is the app's update path driven by security_policy.json, so it also needs human review after the fix.
Deferred items without issues: the Design "main-process refactor follow-up" and the three UX Watch items (app-only label for Check for updates, the policy file's path, the gateway context wording) are all accepted-and-deferred with no linked issue. Fix them here, or open one issue per item and link each one in the description.
The description's Screenshots section is also out of date. It says the channel row is shown read-only, but What changed says it is hidden, and the alt text quotes an older notice string.
CI is green on this head.
— crew-pr-reviewer (r101)
|
Reply to r101 (iamwhatever — CHANGES_REQUESTED) Commit 27112fb addresses all four points. Sticky flag (governance latches on macOS when the window closes): Fixed in Any renderer can set the flag: Fixed. The handler now reads Deferred items without linked issues: Four issues opened and linked in the description:
Screenshots section out of date: Fixed. The description now says the channel row is hidden (not read-only), matching the code condition |
|
Commit d2ee5bf addresses both blocking findings. Opus F1 / GPT F1 (multi-window governance): GPT F2 (no manual update path when policy governed): |
|
Commit 381d432 addresses the three new blocking findings. GPT/UX — GPT — GPT — feed-lane.js freshness verification race: Added |
|
Commit ebd69c9 addresses all remaining blocking findings. GPT F1 (download governance race): Added GPT F2 / UX BLOCK (policy notice + controls): UX BLOCK (gateway switch ambiguity in topology 2):
|
|
Commit 85a13cf — pseudolocale regenerated. The |
|
Commit 6ae6e32 fixes both new blocking findings. Opus BLOCK (duplicate GatewayAutoUpdateSwitch): Passed GPT F1 (download-progress badge stuck after governance): Added |
|
Commit 32817c1 addresses GPT F1 and F2. GPT F2 (governance during stopGateway): Added
GPT F1 (remote bundled gateway suppressing local app): Added |
|
Commit 769a6b5 addresses all remaining blocking findings. GPT F1 (SSH tunnel with local gateway enabled): The GPT F2 (governance during stopGateway): GPT F2b (managed lane + policy): |
|
Commit 7420cbc fixes the Electron Shell Tests failure.
|
|
Commit 164e1f0 fixes both new blocking findings. GPT F1 (managed-lane.js governance during stopGateway): Added GPT F2 (ipc-registrar.js destroyed sender after port probe): Added Also in this commit: async race per-sender sequence tracking ( |
…bundled installs Two topologies let the gateway's security policy provider and the Electron app's own updater act simultaneously on the same bundled install: 1. **Policy provider on a desktop install.** When a security_policy.json `updates` block configures `check_command`/`apply_command`, the gateway runs the provider; the Electron auto-updater also checks independently. Both can find and apply an update at the same time. Per governance.md, the provider owns the check, the badge and the Update button and is resolved before any layout's own updater or deferral: the fix makes the Electron feed lane stand down when the gateway reports the provider governs. 2. **Reused gateway (CLI service, SSH tunnel).** The About page's gateway auto-update switch was hidden in a desktop window when the effect was unknown, because `update_bundled_by_app` was not yet used to tell the bundled gateway apart from a reused one. Changes: - `website/electron/runtime/update/feed-lane.js`: add `isPolicyGoverned` parameter; `safeCheck()` returns early with `not-available` when governed, so the Electron auto-updater does not contact the feed or fire a notification. - `website/electron/auto-update.js`: add `isPolicyGoverned` parameter; pass it to `createFeedLane`. - `website/electron/ipc-registrar.js`: add `policyGoverned` variable; add `update:notify-policy-governed` IPC handler; wire `isPolicyGoverned`. - `website/electron/preload.js`: expose `notifyPolicyGoverned` on `updateAPI`. - `website/src/types/electron-bridge.d.ts`: add `notifyPolicyGoverned` to `UpdateAPI`. - `website/src/hooks/useUpdateSubscription.ts`: add `usePolicyGovernedNotification` hook that calls `notifyPolicyGoverned` whenever `update_bundled_by_app && update_managed_by === 'command'` changes. - `website/src/shell/appShell.tsx`: import and call `usePolicyGovernedNotification` after `useUpdateSubscription` (moved from App.tsx due to shell refactoring). - `website/src/pages/settings/AboutPanel.tsx` (policy-managed branch): render `AboutPanelGatewayUpdates` alongside the policy notice so a manual update path remains available even when the Electron feed lane stands down. - `website/src/components/UpdateFoundModal.tsx`: gate the desktop candidate on `!gwPolicyManaged` so the modal does not interrupt a bundled install whose updates the policy provider owns. - `website/src/pages/settings/AboutPanel.tsx`: when `isGatewayPolicyManaged`, replace the desktop update card with a "managed by policy" notice. - `website/src/utils/updateSwitches.ts` (topology 2): `updateSwitchesShown` accepts optional `bundledByApp` from the status frame; the gateway switch is shown when `bundledByApp === false` (a reused gateway that manages its own bytes and is not covered by the Electron updater).
|
HOLD at 76dc554, reserved for human review. Our r101 items are addressed. The governance handler now clears on window destroy, accepts only senders that pass the shared local-dashboard gates plus a current-bundle check (ipc-registrar.js#L708-L804), and the four deferred items are #23871-#23874. CI is green. What blocks:
What clears it: one reply per lane item on this head, the description rewritten from the current diff with fresh screenshots, and a human approval. I did not run the Electron or website tests. — crew-pr-reviewer (r126) |
Problem / Motivation
Goal: (a) Make exactly one updater report on a bundled desktop install — the one governance.md names, so the Electron feed lane stands down when a policy provider can apply; (b) show the gateway's auto-update switch in a desktop window when the connected gateway is a reused one (not the bundled one), so users of a CLI-service or SSH-tunnel gateway can manage its
auto_updatesetting.Two topologies let both the gateway's security policy provider and the Electron app's own updater act simultaneously on the same bundled install:
Policy provider on a desktop install. When a
security_policy.jsonupdatesblock configurescheck_command/apply_command, the gateway runs the provider check (per governance.md: "the provider owns the check, the badge and the Update button, and is resolved before any layout's own updater or deferral"); the Electron auto-updater also checks independently. Both can find and try to apply an update at the same time.Reused gateway (CLI service, SSH tunnel). The About page's gateway auto-update switch was hidden in a desktop window when
update_auto_effectwas not a definiteinstallormandatory, becauseupdate_bundled_by_app(already present in the status frame and the type definition) was not yet used to tell the bundled gateway apart from a reused one.Why it matters
A desktop user with an enterprise policy
updatesblock has two updaters competing for the same bytes. On Windows the policy shell is absent, so the gateway's provider runs nothing while the Electron updater proceeds: neither lane lands reliably.A user whose app connects to a reused CLI-service gateway has no switch to manage that gateway's
auto_updatefrom the About page unless its effect happens to beinstall— a gap that is visible as a missing row on every visit.Not a goal
What changed (motivation → approach → change)
Topology 1 root cause: the Electron feed lane runs independently of the gateway's policy provider. When both compete, the outcome is undefined.
Fix direction (follows governance.md): governance.md records the provider as the single owner; the Electron feed lane must stand down. The renderer already knows the gateway reports
update_managed_by === 'command'andupdate_bundled_by_app === true; the fix plumbs that signal into the main process so the lane can suppress its check.Change:
website/electron/runtime/update/feed-lane.js: addisPolicyGoverned = () => falseparameter;safeCheck()returns early withnot-availablewhen governed, so the Electron auto-updater does not contact the feed, does not start a download, and does not fire a notification.website/electron/auto-update.js: addisPolicyGovernedparameter; pass it tocreateFeedLane.website/electron/ipc-registrar.js: addpolicyGovernedvariable; addupdate:notify-policy-governedIPC handler with origin gate (only accepts from the bundled gateway's own origin, so a window connected to a reused or tunnelled gateway cannot suppress the app's updater) and destroyed-window clearing (registerssender.once("destroyed", …)so closing the window on macOS — where the app keeps running — clears the flag automatically rather than latching it forever); wireisPolicyGoverned: () => policyGovernedtoinitAutoUpdate; triggerupdater.check()on the transition to governed so any stale "found" state is cleared.website/electron/preload.js: exposenotifyPolicyGovernedonupdateAPI.website/src/types/electron-bridge.d.ts: addnotifyPolicyGovernedtoUpdateAPI.website/src/hooks/useUpdateSubscription.ts: addusePolicyGovernedNotificationhook that callsnotifyPolicyGovernedwheneverupdate_bundled_by_app && update_managed_by === 'command' && update_can_apply === truechanges. Theupdate_can_applygate keeps Windows working: on that platform the policy shell is absent andcan_applyis false, so the Electron updater proceeds as the only working update path.website/src/shell/appShell.tsx: import and callusePolicyGovernedNotificationafteruseUpdateSubscription(the shell refactoring moved the hook composition root fromApp.tsxtoappShell.tsx).website/src/components/UpdateFoundModal.tsx: gate the desktop candidate on!gwPolicyManaged(same three-field condition).website/src/pages/settings/AboutPanel.tsx: whenisGatewayPolicyManaged, replace the desktop update card with a policy notice using the newupdates_managed_by_policy_desktopi18n key ("A policy on this computer manages updates for this app. New versions arrive through that policy, so {{productName}} doesn't check for updates itself. To change this, edit security_policy.json or ask whoever set it up."). The original sharedupdates_managed_by_policykey is left byte-identical; only the new desktop-bundled-install notice uses the new key. The circular-arrows icon in the Updates section heading is kept unconditionally; it is decorative and not a button. The channel row is hidden whenisGatewayPolicyManaged(condition!isGatewayPolicyManaged && !isExternallyManaged) because showing a read-only channel next to "doesn't check for updates itself" confused readers who questioned whether the channel still matters.GatewayAutoUpdateSwitchvisibility followsupdateSwitches.gateway; for a bundled install withbundledByApp: true, the gateway row is already hidden. In a desktop window withbundledByApp === false, a context note ("The gateway runs your agents and chats. It's installed separately from {{productName}}, so this switch updates it, not this app. It only restarts when no work is active.") appears above the switch to distinguish it from the app's own update card. Why the existingupdates_managed_by_policykey is not used for this notice: the new key's text is specific to the desktop bundled case: "A policy on this computer manages updates for this app. New versions arrive through that policy, so Kiro Crew doesn't check for updates itself. To change this, edit security_policy.json or ask whoever set it up." The existing key says "operator-defined update policy…the mechanism it defines" — it does not state that Kiro Crew doesn't check for updates, which is the key fact for this code path. UX review confirmed the new text satisfies the bundled-install use case.Topology 2 root cause:
updateSwitchesShownin a desktop window only showed the gateway switch wheneffect === 'install'or'mandatory', and a comment documented the gap: "an unknown effect cannot tell that gateway from one the app merely attaches to."update_bundled_by_appwas already in the status frame and type definition but unused.Change:
website/src/utils/updateSwitches.ts:updateSwitchesShownaccepts optionalbundledByAppparameter; the gateway switch is shown whenbundledByApp === false(a reused gateway that manages its own bytes and is not covered by the Electron updater).useUpdateSwitchesShownreads it from the Redux status slice.Intended? / Repro
Intended? No for topology 1; it is a gap in the plumbing, not a design choice. governance.md §
check_command/apply_commandsays "the provider owns the check, the badge and the Update button, and is resolved before any layout's own updater or deferral", and theUpdateFoundModal.tsxcomment at line 142 (before this fix) already says "the two cannot both report." The two-reporter gap is a missing gate, not a documented behavior. The fix follows governance.md's ownership rule: provider owns the check; Electron feed lane stands down.No for topology 2;
update_bundled_by_appwas already wired into the status frame and type definition specifically to allow this distinction.Repro (by code reading):
initAutoUpdate(ipc-registrar.js) callscreateFeedLanewith no governance input;safeCheck()callsautoUpdater.checkForUpdates()unconditionally. At the same time, the gateway runsresolve_provider()and callscheck_command. Both emitters fire.updateSwitchesShownat commit70d2fa6a:gateway: !bridge || effect === 'install' || effect === 'mandatory'. Whenbridgeexists and effect is'unknown',gatewayisfalse. Theupdate_bundled_by_appfield is never read.Screenshots
Policy-managed bundled install (AFTER, topology 1) — dark mode:
update_bundled_by_app: true,update_managed_by: 'command',update_can_apply: true. The policy notice replaces the update card and Check button. The channel row is hidden (condition!isGatewayPolicyManaged && !isExternallyManaged); showing a read-only channel next to "doesn't check for updates itself" confused readers who questioned whether the channel still matters. The notice reads: "A policy on this computer manages updates for this app. New versions arrive through that policy, so Kiro Crew doesn't check for updates itself. To change this, edit security_policy.json or ask whoever set it up."Policy-managed bundled install — light mode:
Non-governed bundled install (BEFORE / comparison state) — dark mode:
update_managed_by: 'electron'. The regular update card, Check button, and AppAutoDownloadSwitch are present.Reused gateway (AFTER, topology 2) — dark mode:
update_bundled_by_app: false,update_auto_effect: 'install'. The gateway auto-update switch is visible becausebundledByApp === falseinupdateSwitchesShown. A context note inside the gateway section explains: "The gateway is the background service this window connects to. It updates independently from Kiro Crew." The description reads "Restarts the gateway to install a new release once no work is running. When off, it stays on its current version." (no "on installs that can update themselves" qualifier).Backwards compatibility
feed-lane.js:safeCheck()returnsnot-availableearly (without callingautoUpdater.checkForUpdates()) whenisPolicyGoverned()is true. The existingupdate-not-availablehandler already runs in that case and emits the same state it would on a normal "no update" response. The change is safe: the lane only stands down when the renderer has explicitly notified governance; the default is() => false, so no install is affected unless explicitly notified.notifyPolicyGovernedis an optional field onUpdateAPI; older shells don't have it, and the hook call is?.-guarded. ThebundledByAppparameter toupdateSwitchesShownis optional and defaults toundefined, so callers that pass none keep the old behavior.Removes nothing: no API, field, route, or public symbol is removed or renamed.
Tests
Electron —
website/electron/test/auto-update-policy-governed.test.js(new):when policy governs, check() emits not-available without contacting the feed:isPolicyGoverned: () => true; assertsautoUpdater.checkForUpdatesnot called and last emitted state isnot-available.when governance is cleared, the next check contacts the feed:isPolicyGovernedtoggled false; assertscheckForUpdatesis called.Electron —
website/electron/test/ipc-registrar.test.js(updated contract):registerUpdater owns six handlers and preserves lazy gateway hooks: updated to assert one listener (update:notify-policy-governed), verifyisPolicyGovernedis a live closure (not a static boolean), and verify the listener updates the closure.Frontend —
website/src/components/UpdateFoundModal.test.tsx:the desktop candidate is skipped when a policy provider governs a bundled install: store withupdate_bundled_by_app: true, update_managed_by: 'command', update_can_apply: true; desktopfoundstate; asserts modal does not open.Frontend —
website/src/test/AboutPanel.policyManaged.test.tsx(new, 5 cases):bundled + command + can_apply).can_apply: false(Windows — provider shell absent).managed_by !== 'command'.bundled_by_app === false.Frontend —
website/src/utils/updateSwitches.test.ts(topology 2, three tests):draws the gateway row in a desktop window for a reused (non-bundled) gateway:bundledByApp: falsewithnotify/unknowneffects →gateway: true.still hides the gateway row for the bundled gateway:bundledByApp: truewithnotify/unknown→gateway: false.treats an unresolved bundled status conservatively:bundledByApp: undefinedwithnotify→gateway: false.Red without the fix: the Electron tests fail because
checkForUpdatesis called unconditionally. The modal test fails because the desktop candidate is not gated. The About page tests fail becausegateway-policy-managed-noteis not rendered. TheupdateSwitchestests fail becausebundledByAppis not in the condition.Related Issues
Fixes #15797
Follow-up issues (accepted-and-deferred)
security_policy.jsonpath in the policy-managed notice (UX watch item)Pattern harvest
Rule candidate: review-prompt
Pattern: a second lane that fires on the same install as a first, controlled lane — the first lane's ownership is documented but the second's suppression gate was never wired.
Checklist
Contribution License Agreement
Contributed under the project's Apache-2.0 license.