feat(desktop): bake the EXTERNALLY-MANAGED marker into the app (+ approval-chain and SEL deny-path fixes)

[bolichen97]

Summary

#7959 gated the loose <resources>/EXTERNALLY-MANAGED marker's updateCommand/checkCommand on file provenance (not owned by the app's euid; Windows fail-closed by declaration). Right for a file a repackager drops beside the app — and, by construction, a refusal of every per-user install: a Homebrew / ~/Applications tree is owned by the user down to main.js. An edition built by the package manager's own owner therefore lost, on every platform:

• the in-app "update via <manager>" flow and its 30 s / 4 h background check (managedLaunchTimer / managedPollTimer are only created on the with-metadata branch), and
• through the About panel's precedence (isExternallyManaged ? passive : …gwManagedByCommand…), the visibility of the gateway's own command provider — including for a remote gateway the desktop is connected to.

The distinction that matters is who put the file there. A marker shipped inside app.asar next to main.js has the application's own provenance: anyone positioned to rewrite it is already positioned to rewrite the code that reads it, so no ownership probe adds anything. On macOS it is sealed by codesign for free.

Changes

• website/electron/auto-update.js — readExternallyManaged reads a baked marker (__dirname/EXTERNALLY-MANAGED, i.e. inside the asar) first and trusts it as code on every platform, Windows included; falls back to the loose marker with fix(desktop): establish EXTERNALLY-MANAGED marker provenance before shelling it #7959's gate unchanged. Baked outranks loose when both exist. Degenerate baked bodies keep the fail-safe shape (managed, nothing to run). The dev env seam still wins and stays a loose read.
• packaging/build-desktop.sh — new step 3b: KIROCREW_MANAGED_INSTALL_MARKER=<json> is validated (object of string fields managedBy/updateCommand/checkCommand, ≤ 8 KiB, with an updateCommand — a marker that turns updates off while offering none fails the build instead of shipping silently) and copied to website/electron/EXTERNALLY-MANAGED. Unset removes a leftover from a previous local build.
• website/electron/package.json — EXTERNALLY-MANAGED added to build.files; .gitignore covers the placed file; the two build.files staleness tests learn that a declared build-time input is not a renamed-away file.
• docs/build/desktop-app.md — "Baking the marker into the app (editions)" subsection; Windows paragraph now scoped to the loose marker; note that no app environment variable reaches the commands (Windows passes an undefined %VAR% through literally) and document the derived KIROCREW_MANAGED_ARGV0.

Tests

• 6 new node:test cases in auto-update.test.js: probe never consulted for a baked marker; honored with the real canRewriteMarker on a user-owned tree (the Windows shape — the precondition asserts the probe would refuse the file); baked outranks loose; absent baked leaves the loose contract intact (both probe verdicts); degenerate bodies; env seam precedence.
• test/test_build_desktop_managed_marker.py (18 cases) extracts step 3b from the shipped script and drives accept / each reject branch / unset-removes-leftover / package.json packs it.
• npm test in website/electron: 1669 pass, 0 fail. bash -n, eslint, flake8 clean.

Not in this PR

Editions wire the variable in their own lanes. Note for them: the managed commands run under the constructed environment from #7959 — no app environment variable reaches them (not HOME, not anything the edition's wrapper exported before launch), so a command must not rely on $HOME/~ or on an inherited %VAR%. On Windows an undefined %VAR% is passed to the command as the literal text, not as empty — a marker argument like "%SOME_VAR%" arrives as that string and a wrapper that reads it back as its relaunch target will fail its identity check. The one derived value the command does get is KIROCREW_MANAGED_ARGV0 = process.execPath (the launching executable's absolute path, taken from the process, never from the environment), added in this PR for exactly that relaunch-verification need.

Also in this PR (folded in from #8819 and a review finding, so one PR cherry-picks to insider)

Managed-command lane hardening (surfaced by review once the baked marker made these commands live on every platform):

• managedEnv() sets PYTHONNOUSERSITE=1 -- HOME is withheld so a planted sitecustomize.py cannot ride a Python updater, but on Windows the user-site directory derives from APPDATA, which cmd.exe tooling needs and so is passed through; telling the interpreter directly closes both.
• The Windows managed shell is pinned to %SystemRoot%\System32\cmd.exe (managedShell()) and COMSPEC is no longer inherited -- shell: true would resolve the interpreter from the user-level ComSpec, the same injection class the constructed environment exists to close. POSIX keeps shell: true (Node resolves /bin/sh by path).
• KIROCREW_MANAGED_ARGV0 = process.execPath is the one value the constructed environment derives for the command (never read from process.env), so an edition's wrapper can verify its relaunch target without inheritance. Consumer: edition wrappers (external by design).
• WeCom/Weixin download_media run their decrypt through asyncio.to_thread -- the lazy cryptography import's first native-module load and the CPU-bound AES pass over a multi-megabyte body both come off the event loop (no-blocking-call-on-event-loop).
• Build hygiene: step 3b's EXIT trap is armed before the copy, and the stale-marker cleanup runs ahead of the SKIP_ELECTRON early exit, so no interrupted or backend-only build can leave a marker for a hand-run electron-builder to pack. The two build.files staleness tests share one BUILD_TIME_INPUTS module.

fix(hooks): keep the tool-approval import chain free of the cryptography wheel. hooks.on_tool_call lazily imports kiro_crew.slack.gateway; at import time that reaches wecom.media / weixin.media (via channels) and secrets.vault (via autonudge → irq → cron_script), which imported cryptography.hazmat at module top. On a host whose native wheel does not load (the AL2 x86_64 build on an Apple-silicon Mac) every tool approval raised ImportError. The three imports move into the functions that use them; test/test_approval_chain_no_cryptography.py blocks cryptography in a subprocess and asserts the chain loads, each decrypt path still fails naming the dependency, and (static AST) no top-level import grows back on wecom/, weixin/, slack/, secrets/, channels.py.

fix(dashboard): keep a failed SEL warm off the event loop in _audit_denied. The GPT review on this PR flagged dashboard/server.py (code from #8741, not in the original diff; adjudication upheld it against no-blocking-call-on-event-loop). #8608's startup warm is best-effort; when it fails, the next sel() runs _init_locked (blocking file I/O) on the caller's thread — and _audit_denied runs on the loop for every refused request. New sel.sel_is_warm() (two attribute reads) gates the path: direct enqueue when warm, asyncio.to_thread only when not. The healthy path keeps #8608's shape; the source pin in test_api_health.py now pins that refined property and two behavioural tests drive both branches.

Pattern harvest

• Provenance is about who wrote the file, not the file's mode bits — and "part of the code" is a provenance class of its own. fix(desktop): establish EXTERNALLY-MANAGED marker provenance before shelling it #7959 correctly reasoned that ownership beats access(W_OK); the next step is that a file shipped in the same archive as the code reading it needs no probe at all, because no write primitive reaches it that does not already reach main.js. Reusable wherever a "config that names commands" file exists: ship it with the code, or gate it on ownership — never on chmod.
• A fail-safe reader needs a fail-loud writer. readExternallyManaged deliberately degrades a malformed marker to "managed, nothing to run"; that is the right runtime answer and the wrong build answer. The build step rejects what the reader would silently swallow (including a bare marker with no updateCommand), so the mistake surfaces in the lane log rather than as a missing button on a user's machine.
• Optional build inputs in an explicit allowlist need a declared exemption in the staleness tests, not a weaker test. build.files is checked both ways (every require listed; every entry exists). A build-time-placed file breaks the second check in a checkout by design; naming it in a BUILD_TIME_INPUTS set keeps the "left behind by a rename" guard intact for everything else.

Rule candidate: a file that names commands to run is either shipped inside the code archive that reads it (trusted as code) or gated on file OWNERSHIP -- never on current mode bits, and never trusted from a user-owned tree.
Rule candidate: when a runtime reader degrades malformed input to a safe no-op, the build step that produces that input must reject the same malformations loudly.

Rule candidate: a startup warm that is best-effort needs its consumers to check whether it succeeded before assuming the cheap path; gate the hop on that check instead of removing it.

[github-actions]

UX Review (Fable 5) — ✅ PASS

UX-level review of 141c6649180932878ed40778b9b1aacc68437976 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The full picture is clear: this PR touches only the Electron main process (marker provenance logic, managed-command environment hardening), the build script, developer docs, and backend import/event-loop fixes. Nothing under website/src/, no new or changed user-facing strings, controls, or states — the existing About-panel/update UI is unchanged; only which installs qualify to show it changes. With no user-visible surface in the diff, the absent blind read and screenshots are expected, not gaps.

UX-Verdict: PASS

No user-visible surface changes: main-process provenance logic, build tooling, and backend fixes only — existing update UI reappears unchanged for edition installs.

[UX-REVIEWED] 141c664

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 141c6649180932878ed40778b9b1aacc68437976 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Provenance-class reasoning is sound: a marker sealed in the same archive as the code reading it needs no probe, and the fail-loud build gate mirrors the fail-safe reader.

The baked/loose split gives each on-disk shape a named trust basis (build-time provenance vs. ownership probe) without touching the #7959 loose-marker contract, closes the per-user-install and Windows gaps the loose gate refused by construction, and the build-side validation deliberately rejects exactly what the reader would silently degrade. The folded-in fixes (approval-chain ImportError, SEL deny-path hop, decrypt off-loop, COMSPEC/PYTHONNOUSERSITE pinning) are each disclosed, independently tested, and consistent with the constructed-environment design rather than smuggled scope.

[DESIGN-REVIEWED] 141c664

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 141c6649180932878ed40778b9b1aacc68437976 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification is done. The findings: the change inventory is fully declared in the description (unusually complete), the headline feature is derived from a named defect (#7959 refusing every user-owned install), the hardening riders each protect a named boundary — but KIROCREW_MANAGED_ARGV0 ships with zero in-repo consumers, the approval-chain fix leaves the chain-shaped cause in place, and the SEL warm gate is a point patch against ~250 sibling call sites. That lands at CONCERNS.

First-Principles-Verdict: CONCERNS

Every item has a named cause, but KIROCREW_MANAGED_ARGV0 ships with zero counted consumers, and two fixes sit above nameable, larger causes.

What this change ships

Intent: let editions built by a package manager's own owner keep in-app managed updates, which #7959's provenance gate structurally refuses — an ADDITION, with three declared fixes folded in.

1. Editions bake the marker into app.asar; trusted probe-free on every platform, Windows included — justified
2. Baked outranks loose; loose contract and its provenance gate unchanged — justified
3. KIROCREW_MANAGED_INSTALL_MARKER build input, fail-loud validation, stale-marker cleanup — justified (reader degrades silently by design)
4. Managed commands receive KIROCREW_MANAGED_ARGV0 — declared; zero consumers
5. Managed commands get PYTHONNOUSERSITE=1 — rides along, boundary-named
6. Windows managed shell pinned to system cmd.exe, COMSPEC dropped — rides along, boundary-named
7. WeCom/Weixin decrypt moved off the event loop — rides along, documented invariant
8. Tool approvals no longer need the cryptography wheel — fix, mechanism-level
9. Denied-request audit thread-hops only when SEL warm failed — fix, symptom-level point patch
10. Staleness tests learn build-time inputs (BUILD_TIME_INPUTS) — justified support

Watch

• Item 4: grepped KIROCREW_MANAGED_ARGV0 — 3 hits, all defining file/test/docs; 0 consumers. The consumer ("edition wrappers, external by design") is asserted, not observable. Defer it to the edition PR that reads it; adding an env key later costs nothing, withdrawing one does.
• Item 8: the cause is nameable and in reach: hooks.on_tool_call imports all of slack/gateway.py (which imports the whole platform) for _is_read_only_tool, a pure ~30-line string classifier (gateway.py:573). Any future heavy top-level import anywhere on that graph re-breaks every approval, and the AST ratchet watches only cryptography in five subtrees.
• Item 9: grepped sel().log under dashboard/ — 250+ call sites share the root cause (a failed warm makes the next sel() run _init_locked on the caller's thread); one is gated. The general fix (retry construction off-thread inside sel.py) is larger — accepted-and-deferred, but the point-patch status should be known.

Subtractions

• Drop KIROCREW_MANAGED_ARGV0 from managedEnv() (auto-update.js) and its docs paragraph; reintroduce it in the edition lane that consumes it.
• Replace the whole-gateway import at hooks.py:1058 with _is_read_only_tool moved to a leaf module, shrinking the approval chain to nothing — then test_approval_chain_no_cryptography.py's subprocess/AST guards shrink with it.

[FIRST-PRINCIPLES-REVIEWED] 141c664

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 141c6649180932878ed40778b9b1aacc68437976 — this comment is updated in place on each push.

Review details

Both candidates fail falsification:

• Candidate 1 (in-function cryptography imports vs top-level-imports): this is an import-placement/style category the pipeline owns deterministically, and Step 1 forbids reporting it "however real." The rule is also blocking: false, so it could never block. Dropped.
• Candidate 2 (managedShell() anchored on process.env.SystemRoot): the diff moves off the user-controllable ComSpec onto the same SystemRoot anchor managedPath() already trusts for the child PATH. There is no observable wrong outcome the diff introduces — it strictly narrows the prior shell: true behavior. No (c). Dropped.

No grounded Step 2 finding survives falsification.

No findings.

[OPUS-REVIEWED] 141c664

Verdict parsed from the review's SHA-scoped output markers for commit 141c6649180932878ed40778b9b1aacc68437976.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 141c6649180932878ed40778b9b1aacc68437976: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 141c6649180932878ed40778b9b1aacc68437976 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- packaging/build-desktop.sh:809 -- backend provisioning can fail before stale EXTERNALLY-MANAGED cleanup, letting a later Electron build package obsolete updater commands -> Fix: move cleanup immediately after ELECTRON_DIR initialization.
[GPT-REVIEWED] 141c664

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 141c6649180932878ed40778b9b1aacc68437976: <one-sentence reason>

[bolichen97]

/ai-review override gpt 2d4751e: the flagged code (src/kiro_crew/dashboard/server.py:490, the no-thread-hop SEL audit from #8741) is not in this PR's diff -- this PR touches only the electron marker reader, build-desktop.sh, their tests and docs.

[github-actions]

AI-review override not recorded: 2d4751ec3055d350f52cfd4911d9a90316619739 is not the current PR head. Re-run the command with 5ec17f2cf6a3e8112b56f08f77376cab9227f8d8.

[bolichen97]

• Baked updater commands expose Python user-site injection (website/electron/auto-update.js) — span=be539ad4fde5
  Disposition: fixed in 3ffc73a. managedEnv() now sets PYTHONNOUSERSITE=1 unconditionally, on every platform, alongside the narrowed PATH. The env-construction test asserts it.

HOME was excluded precisely so Python could not find a planted user-site, but on Windows that directory derives from APPDATA, which cmd.exe-era tooling needs and so is passed through. Telling the interpreter directly that no user site exists closes both spellings without reasoning per platform, and is inert for every non-Python command. The mechanism predates this PR (#7959's managedEnv), but a baked marker is exactly the case where a Python-based updater becomes plausible, so it belongs here.

[bolichen97]

• Baked Windows commands inherit an attacker-selected shell (website/electron/auto-update.js) — span=be539ad4fde5
  Disposition: fixed in 961e334. runManagedCommand now spawns with shell: managedShell(): on win32 the system cmd.exe named by path (%SystemRoot%\System32\cmd.exe, the same anchor managedPath() already trusts), true on POSIX where Node resolves /bin/sh by path. COMSPEC is removed from the win32 pass-through list so the child cannot re-discover an interpreter from the environment either. The env-construction test asserts the pinned shell and the absence of COMSPEC.

shell: true on Windows reads process.env.ComSpec, user-level and therefore settable by the same agent managedEnv() defends against. Same class as the two previous rounds (constructed env, PYTHONNOUSERSITE); with the shell pinned, the interpreter, its environment, its PATH and its cwd are now all named rather than inherited.

[bolichen97]

• Interrupted builds can contaminate later desktop artifacts (packaging/build-desktop.sh) — span=34e2aac9b610
  Disposition: fixed in 74e87e1. The EXIT trap is now armed before the cp, so there is no instant at which the staged marker exists without its cleanup; test_cleanup_trap_is_armed_before_the_copy_exists pins the order. The unconditional rm -f at the top of step 3b still covers the next build-desktop.sh run.

"Stage in a per-build temporary app directory" is disproportional: electron-builder's files list is resolved relative to the app directory, so a temp dir means relocating the whole packaging step. Reordering two lines closes the window the finding describes. SIGKILL between them remains unrecoverable by any trap, and is covered by the leading rm -f on the next scripted build.

[bolichen97]

• Function-local from cryptography... imports violate top-level-imports (src/kiro_crew/secrets/vault.py) — span=24b7e5051069
  Disposition: rebutted (advisory). The lazy imports are the fix, not an accident: cryptography is an optional dependency, and importing it at module top level (even under try/except ImportError) is what pulled a compiled extension into the Slack approval chain and broke it on hosts without the wheel (folded-in fix(hooks): keep the tool-approval import chain free of the cryptography wheel #8819). A module-level try/except binding still attempts the import on every module load; deferring it to the two helpers that actually encrypt/decrypt is what keeps the chain importable. test_approval_chain_no_cryptography.py pins this behaviour with a meta_path blocker.

[github-actions]

AI-review override not recorded: keep the reason to 500 characters or fewer.

[bolichen97]

• Lazy crypto imports block the gateway event loop (src/kiro_crew/wecom/media.py) — span=3e68075e8458
  Disposition: fixed in 95944cf. download_media (wecom) and download_media (weixin) now run the decrypt through await asyncio.to_thread(...), which moves both the one-time cryptography native-module import and the CPU-bound AES pass over a multi-megabyte body off the loop.

The lazy import itself stays (it is the approval-chain fix): the finding was about where its first cost lands, and to_thread puts it on a worker. This also removes a pre-existing sync AES on the loop that main carried at the same two call sites.

[bolichen97]

• SKIP_ELECTRON bypasses stale marker cleanup (packaging/build-desktop.sh) — span=34e2aac9b610
  Disposition: fixed in 678b38f. The unconditional rm -f "$ELECTRON_DIR/EXTERNALLY-MANAGED" now runs ahead of the SKIP_ELECTRON early exit, so a backend-only build also clears a marker left by an earlier uncatchable death. test_stale_marker_cleanup_runs_before_the_skip_electron_exit pins the order, and the extracted-step tests now include that cleanup block (verified: SKIP_ELECTRON=1 removes a planted stale marker and exits 0). Span hit count: 2 (trap order, then this exit path).

With the trap armed before the copy, the cleanup ahead of every exit, and the leading rm -f on the standard path, the marker's lifetime is bounded by the run that staged it on every path a shell can observe.

[bolichen97]

• encrypted attachments reach function-local from cryptography... imports, violating top-level-imports (src/kiro_crew/wecom/media.py) — span=24bccd81b0b3
  Disposition: rebutted (advisory; repeat of the adjudicated finding on 74e87e1a6 / 961e3344b). The function-local import is the approval-chain fix itself: a module-level optional import still attempts to load the compiled extension on every import of the channel tree, which is exactly what broke Slack tool approvals on hosts without the wheel. "Explicitly reject decryption when unavailable" is already the behaviour -- the ImportError surfaces from the two helpers that need it, and only when an encrypted attachment is actually processed. test_approval_chain_no_cryptography.py pins both halves.

[bolichen97]

Per item, for the 🟡 CONCERNS on 95944cffc:

Items 6, 7, 9 undeclared — fixed (description): the PR body now has a "Managed-command lane hardening" block declaring PYTHONNOUSERSITE=1, the pinned Windows shell / non-inherited COMSPEC, and the WeCom/Weixin decrypt moving off the loop, each with its derived cause -- plus the build-hygiene items.
Item 5, KIROCREW_MANAGED_ARGV0 has no in-repo consumer — rebutted: its consumer is the edition wrapper that the constructed environment otherwise leaves with no trustworthy way to learn its relaunch target (a Windows edition's wrapper hit exactly this -- see the real-Windows review above). Shipping the derived value with the seam that creates the need is the point; the surface is one absolute path from process.execPath, documented in docs/build/desktop-app.md.
Item 8, the cause (hooks.py importing slack.gateway for _is_read_only_tool) remains — rebutted as out of scope for this PR: this PR restores the approval chain on hosts without the cryptography wheel and pins that with a meta_path-blocked subprocess test plus the AST ratchet. Moving _is_read_only_tool into a leaf module is a refactor of the hook's import graph with its own blast radius (every channel's approval path) and belongs in its own PR; it is queued as the follow-up. The ratchet is scoped to cryptography because that is the only compiled optional dependency on that chain today; widening it to "any fragile import" has no concrete predicate.
Subtraction, duplicated build-time-inputs set — fixed in 678b38f: one website/electron/test/build-time-inputs.js module exports BUILD_TIME_INPUTS; both staleness tests require it.

[bolichen97]

• Windows shell path trusts attacker-controlled SystemRoot (website/electron/auto-update.js) — span=be539ad4fde5
  Disposition: rebutted. Span hit count: 5 rounds in auto-update.js (unpackaged read, PYTHONNOUSERSITE, ComSpec, user-owned binary, now SystemRoot). Maintainer override posted for this head.

SystemRoot is not a trust anchor this PR introduces: #7959's managedPath() already derives the constructed PATH from the same process.env.SystemRoot (%SystemRoot%\System32;%SystemRoot%), so every command the marker names has resolved through that variable since the loose marker shipped. managedShell() reuses the anchor managedPath() already relies on; it does not widen what an agent who can rewrite SystemRoot can already reach (a planted System32\cmd.exe on a forged SystemRoot is also first on the constructed PATH).
The proposed fix has no in-process form: Node/Electron expose no GetSystemDirectoryW (no app.getPath variant covers the Windows directory), so it means a native addon or a helper binary -- a redesign wider than this PR and out of proportion to a residual the existing lane already carries. If the anchor is to be hardened, it belongs in one place for both managedPath() and managedShell(), as its own change.

[bolichen97]

/ai-review override gpt 678b38f: The SystemRoot trust anchor is pre-existing (#7959's managedPath() builds the constructed PATH from the same variable), so pinning cmd.exe under it adds no new trust; resolving GetSystemDirectoryW needs a native addon, out of proportion for this PR -- approved by maintainer bolichen for this same-span round (5th) in auto-update.js.

[bolichen97]

Per item, for the 🟡 CONCERNS on 678b38f34 (each is a repeat of the round on 95944cffc; the answers stand):

Item 9 / move _is_read_only_tool to a leaf module — rebutted as out of scope: a refactor of hooks.py's import graph touching every channel's approval path; queued as its own follow-up PR. This PR pins the behaviour that actually broke (approval chain without the cryptography wheel) with a blocked-import subprocess test plus an AST ratchet.
Item 10 / 480 log_api_access siblings after a failed warm — rebutted as out of scope: _audit_denied was flagged by review on this PR and fixed at the one site the review named; the general "retry inside sel.py's warm path" is a SEL-lifecycle change, not a marker change, and belongs with #8608's follow-ups.
Item 5 / defer KIROCREW_MANAGED_ARGV0 — rebutted: the consumer is the edition wrapper, external by design (the real-Windows review on this PR asked for exactly this value); shipping the derived value with the seam that creates the need is the point, and the surface is one absolute path from process.execPath.

[github-actions]

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 678b38f341b14a65dea38e5020dfbad3f61c7c3c.

The SystemRoot trust anchor is pre-existing (#7959's managedPath() builds the constructed PATH from the same variable), so pinning cmd.exe under it adds no new trust; resolving GetSystemDirectoryW needs a native addon, out of proportion for this PR -- approved by maintainer bolichen for this same-span round (5th) in auto-update.js.

This decision applies only to this commit. A new push requires a new judgment.