fix(windows): settle scheduler verification and record Bun runtime provenance - #887
fix(windows): settle scheduler verification and record Bun runtime provenance#887lidge-jun wants to merge 5 commits into
Conversation
…llback An elevated schtasks /create can finish before the non-elevated view catches up. finalize verified exactly once, so a task that was merely not visible yet read as 'not installed' and got rolled back milliseconds before Task Scheduler would have listed it. Verification now re-checks on a bounded 1.1s backoff, but only while the failure looks like a lagging view: the task is not visible, or it is visible without a fully published registration. Every other verdict keeps its meaning and spends no delay at all. Proven WinSW presence is rejected independently of conflict, because conflict only becomes true once the task itself is visible -- while it is still invisible the pair is conflict:false with nativeServiceAbsent:false, and retrying that would wait for a service that is already proven present. Rollback deletes a real task, so it now takes the same ownership fence the state write already had; a stale attempt can no longer delete a task a newer attempt owns. Eight regressions cover the settle path and each fail-closed class; four of them fail without this change.
doctor kept telling Windows users to set OPENCODEX_BUN_PATH even when the override was already active. The payload it reads had a Bun version but no runtime origin, so the auto-known-bad branch could not tell an override from the bundled binary and printed the same remedy either way. Origin cannot be recovered after the fact: resolving it at report time answers 'what would this shell pick now', which is a different question from 'what was the service started with', and the two diverge exactly when it matters. So the selecting launcher now stamps it. Every real launch path carries the marker -- npm launcher, scheduler wrapper, WinSW, launchd, systemd, plus the Codex autostart shim and the tray host, which relaunch the proxy themselves and would otherwise erase it. Path and provenance come from one resolution at each site, so the marker cannot describe a binary other than the one baked. Read-back allowlists the three values and never falls back to resolving locally. A service installed before the marker existed reports nothing, and doctor says the origin is unknown instead of guessing -- an absent marker is an answer, not a gap to fill. bunRevision stays informational and the conservative auto-known-bad result for canaries is untouched; bun-stream-caps.ts and responses/core.ts are absent from this diff. Fixes #848.
Review found the marker stopped at the launchers that resolve a binary. The ones that re-exec process.execPath -- ocx ensure, GUI/Claude/OpenCode start, POST /api/system/restart, the update relaunch -- copied the parent environment and handed the daemon nothing, so a service the launcher knew the origin of still reported unknown. withProcessRuntimeProvenance() covers those seven sites. Re-execing the current runtime does not change how that runtime was obtained, so an inherited marker is preserved and only its absence records 'process'; an unrecognized inherited value is replaced rather than forwarded. The shim builders no longer default provenance to 'bundled'. A default let a caller pass an override binary and label it something else, which is the path/marker disagreement the marker exists to prevent, so the argument is now required. Regressions: the launch sites are pinned so a future launcher that copies process.env cannot silently drop the marker again, and the npm launcher's transport is asserted inside the spawn env rather than inferred from the resolver's return shape. Reverting bin/ocx.mjs to its pre-provenance state fails that test. structure/05: the provenance section had been inserted between the sidebar stop-button heading and its own paragraph; restored.
Review caught the marker leaking. A shim wraps the real codex, so exporting the marker into the shim's own environment handed it to Codex and everything Codex spawns. A shell beneath that running a different Bun directly would carry a provenance describing a binary it was not executing -- and the execPath relaunch paths would faithfully preserve that contradiction into the daemon. Every flavor now scopes it to the ensure invocation: a one-shot assignment prefix in sh, a setlocal/endlocal pair in cmd, and save/restore in PowerShell. Nothing downstream of the shim inherits it. Inheritance is also no longer trusted on its own. withProcessRuntimeProvenance carries a claim forward only when re-resolving it still lands on process.execPath; a stale marker from an ancestor falls back to what this executable actually is. That keeps a genuine restart labelled correctly without letting a marker outlive the binary it was minted for. Also fixes an old-signature caller in tests/openai-provider-option-tooling that passed the token path where provenance now goes -- it wrote a filesystem path into the marker and silently used the default token file, so its sentinel assertion was partly vacuous.
The corroboration added last round asked the wrong question. It re-derived the original selection to decide whether an inherited marker was still true, but a service installed with a shell-local override keeps neither that shell nor its OPENCODEX_BUN_PATH -- so a correct 'override' was demoted to 'process' on the service's first relaunch, and doctor could again suggest setting an override that was already in use. The marker now carries the binary it was minted for. OCX_BUN_RUNTIME_PATH is stamped beside the source at every launcher, and a relaunch keeps the claim when that recorded path is the executable about to run. No re-derivation, so a launcher's own environment no longer has to survive for its provenance to. Comparison goes through realpath, so symlinks, junctions, and Windows case differences no longer reject a valid match. The fallback resolves path and source from one durableBunRuntime() call rather than two, which was a small window where the pair could disagree. Full suite 7030 pass / 0 fail; bun-stream-caps.ts and responses/core.ts remain absent from every commit in this unit.
📝 WalkthroughWalkthroughThe change adds Bun runtime provenance tracking across launchers, services, shims, tray processes, management responses, and diagnostics. It also adds bounded Windows scheduler verification retries with ownership-safe rollback and state handling. ChangesBun runtime provenance
Windows scheduler verification
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ocx.mjs
participant bun-runtime
participant ServiceLauncher
participant ManagementAPI
ocx.mjs->>bun-runtime: resolve Bun path and source
bun-runtime-->>ocx.mjs: validated runtime metadata
ocx.mjs->>ServiceLauncher: spawn with provenance environment
ServiceLauncher->>ManagementAPI: report allowlisted bunRuntimeSource
sequenceDiagram
participant WindowsElevation
participant TaskScheduler
participant Verification
participant InstallState
WindowsElevation->>TaskScheduler: create task
WindowsElevation->>Verification: verify registration
Verification->>TaskScheduler: retry transient visibility
Verification-->>WindowsElevation: verified or fail-closed result
WindowsElevation->>InstallState: write state if ownership remains
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a3bbbe45e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const raw = env[BUN_RUNTIME_SOURCE_ENV]?.trim(); | ||
| return BUN_RUNTIME_SOURCES.find(source => source === raw); |
There was a problem hiding this comment.
When a process inherits OCX_BUN_RUNTIME_SOURCE but subsequently runs a different Bun binary, this function still reports the stale source because it never checks the paired OCX_BUN_RUNTIME_PATH. That is the stale-marker scenario the new withProcessRuntimeProvenance() logic and structure/05_gui-and-management-api.md:120-136 explicitly handle; the management endpoint can therefore tell ocx doctor that an override is active even though the service is actually using a bundled/process runtime. Validate that the recorded path resolves to process.execPath before returning the source, otherwise return undefined.
Useful? React with 👍 / 👎.
| if (d.bunRuntimeSource === "override") { | ||
| lines.push(` OPENCODEX_BUN_PATH is already active for this service — the override runtime is itself an affected version (unvalidated — own risk).`); | ||
| lines.push(" Options: point the override at a different runtime, or opt into streamMode \"eager-relay\" via PUT /api/settings (crash risk on this runtime; see docs)."); | ||
| } else if (d.bunRuntimeSource === undefined) { | ||
| lines.push(" this service records no runtime origin (installed before provenance tracking), so OpenCodex cannot tell whether an override is already active."); | ||
| lines.push(" Reinstall the service to record it, or opt into streamMode \"eager-relay\" via PUT /api/settings (crash risk on this runtime; see docs)."); |
There was a problem hiding this comment.
This changes the user-visible Windows memory workflow: doctor can now say that an override is already active or that a legacy service must be reinstalled, but docs-site/src/content/docs/troubleshooting/windows-memory.md:38-86 and its translations still describe only the previous blanket override workflow and do not explain the provenance/legacy-service message. Update the public troubleshooting page and keep the translated versions consistent so users can act on the new diagnostic.
AGENTS.md reference: AGENTS.md:L200-L201
Useful? React with 👍 / 👎.
| const origin = d.bunRuntimeSource === "process" ? "the runtime that launched it" : "the bundled runtime"; | ||
| lines.push(` the service is using ${origin}. Options: wait for a bundled runtime update, or set OPENCODEX_BUN_PATH to a runtime you trust (unvalidated — own risk),`); |
There was a problem hiding this comment.
When bunRuntimeSource is process and the runtime is affected, this branch still recommends waiting for a bundled runtime update. A process service has baked process.execPath precisely because no bundled runtime was selected, and an opencodex bundled-runtime update does not replace that executable or change the existing service definition, so following this option leaves the affected service unchanged. Give process-origin users remediation for the runtime that launched the service (or tell them to reinstall with a bundled/override runtime) instead.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260802_wt5_windows_service_doctor/000_plan.md`:
- Around line 10-13: The Bug A root-cause statement in the plan must say the old
verifier performed one check and rolled back immediately instead of retrying
transient task-visibility or registration-health failures. Keep the proposed
retry scope limited to those transient states, while preserving fail-closed
handling for conflicts, missing assets, and unknown WinSW status.
In `@src/service.ts`:
- Around line 985-1043: Update schedulerVerificationMaySettle to allow retries
when the task is not installed and nativeStatusUnknown is true, without retrying
proven WinSW presence or an installed task with unknown status. Preserve
existing conflict and asset checks, and add a regression test covering this
compound case in the Windows elevation spawn tests.
In `@src/tray/windows-tray.ps1`:
- Around line 6-8: Update the environment propagation logic in windows-tray.ps1
to add an else branch for an empty $BunRuntimeSource that removes both inherited
Bun provenance keys before launching the child. Preserve the existing
explicit-source behavior, and ensure stale values from process.env cannot reach
reportedBunRuntimeSource().
In `@tests/bun-runtime.test.ts`:
- Around line 152-206: Add regression coverage in the
withProcessRuntimeProvenance tests for both samePath behaviors: create a real
temporary file and symlink alias with fs.symlinkSync, then verify an inherited
marker using the alias is preserved when it resolves to process.execPath; also
exercise the win32 case-insensitive branch by temporarily stubbing
process.platform, asserting differently cased equivalent paths compare as equal,
and restoring the platform value in a finally block.
In `@tests/windows-elevation-spawn.test.ts`:
- Around line 963-986: The test “ownership lost around a non-retryable failure
skips rollback too” must use a genuinely non-retryable verification result, such
as unhealthy assets or a conflict, while still revoking ownership and asserting
rollback is not launched. Add a separate test for the successful path where
verify returns okVerify() after revoking ownership, and assert writeInstallState
is not called to cover the final verification-to-write ownership fence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8562355b-1bd8-468d-972f-c0bfed317fbe
📒 Files selected for processing (27)
bin/ocx.mjsdevlog/_plan/260802_wt5_windows_service_doctor/000_plan.mddevlog/_plan/260802_wt5_windows_service_doctor/010_implementation.mdsrc/cli/claude.tssrc/cli/doctor.tssrc/cli/index.tssrc/cli/opencode.tssrc/codex/shim.tssrc/lib/bun-runtime.tssrc/lib/winsw.tssrc/server/management/system-restart.tssrc/server/management/system-routes.tssrc/service.tssrc/tray/windows-tray.ps1src/tray/windows.tssrc/update/index.tsstructure/05_gui-and-management-api.mdtests/bun-runtime.test.tstests/codex-shim.test.tstests/doctor.test.tstests/memory-watchdog.test.tstests/ocx-launcher-source.test.tstests/openai-provider-option-tooling.test.tstests/service.test.tstests/windows-elevation-spawn.test.tstests/windows-tray.test.tstests/winsw.test.ts
| ### Bug A — PR #868: scheduler registration verification never settles | ||
|
|
||
| - Root cause (from PR body, re-verify): post-create Task Scheduler verification retried non-transient states, so registration verification could fail to settle. Fix retries only transient post-create visibility/XML health states, preserves fail-closed behavior for conflicts/missing assets/unknown SCM status, and stops late reconciliation when attempt ownership changes. | ||
| - Grounding: `src/service.ts`, `src/lib/winsw.ts`, `src/lib/windows-elevation.ts`; verification contracts in scheduler/startup/install tests (PR claims 136 focused tests). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the scheduler root-cause statement.
Line 12 says that the old verifier retried non-transient states. The measured analysis in devlog/_plan/260802_wt5_windows_service_doctor/010_implementation.md lines 14-21 says that it verified once and rolled back immediately. These are different failure mechanisms.
State that the old flow did not retry transient task-visibility or registration-health failures. Keep the new retry scope limited to those transient states. Otherwise, this plan can lead to retries for conflicts, missing assets, or unknown WinSW status, which must fail closed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260802_wt5_windows_service_doctor/000_plan.md` around lines 10 -
13, The Bug A root-cause statement in the plan must say the old verifier
performed one check and rolled back immediately instead of retrying transient
task-visibility or registration-health failures. Keep the proposed retry scope
limited to those transient states, while preserving fail-closed handling for
conflicts, missing assets, and unknown WinSW status.
| /** | ||
| * Bounded post-create backoff, 1.1s total. Task Scheduler's non-elevated view can | ||
| * lag an elevated `/create` by a few hundred milliseconds, so a single verification | ||
| * would roll back a task that is merely not visible yet. | ||
| */ | ||
| const SCHEDULER_SETTLE_DELAYS_MS = [50, 150, 300, 600] as const; | ||
|
|
||
| /** | ||
| * Whether a failed verification is still worth re-checking after a short delay. | ||
| * | ||
| * Retrying is confined to states that a lagging scheduler view actually produces: | ||
| * the task is not visible yet, or it is visible but its registration has not been | ||
| * published in full. Everything else keeps its existing fail-closed meaning and is | ||
| * rejected here so no delay can turn it into a pass: | ||
| * | ||
| * - a proven conflict (both backends present) is a real dual-backend install; | ||
| * - missing assets are missing on disk, which no amount of waiting creates; | ||
| * - a WinSW service that is proven present (`started`/`stopped`) is never absent | ||
| * later. This is checked independently of `conflict`, which only becomes true | ||
| * once the task itself is visible — while the task is still invisible the pair | ||
| * is `conflict: false` with `nativeServiceAbsent: false`, and that must not retry; | ||
| * - unknown SCM status is unproven rather than transient, and has its own | ||
| * task-preserving branch below. | ||
| */ | ||
| function schedulerVerificationMaySettle(v: WindowsSchedulerInstallVerification): boolean { | ||
| if (v.ok) return false; | ||
| if (v.conflict) return false; | ||
| if (!v.assetsHealthy) return false; | ||
| if (!v.nativeServiceAbsent) return false; | ||
| return !v.taskInstalled || !v.registrationHealthy; | ||
| } | ||
|
|
||
| function settleDelay(ms: number): Promise<void> { | ||
| const hook = finalizeHooks?.settleDelay; | ||
| if (hook) return hook(ms); | ||
| return new Promise(resolve => setTimeout(resolve, ms)); | ||
| } | ||
|
|
||
| /** | ||
| * Verify the elevated install, re-checking only while the failure looks like a | ||
| * scheduler view that has not caught up yet. Returns `null` when this attempt lost | ||
| * ownership mid-settle: a newer attempt owns the task, so this one must neither | ||
| * write install state nor roll anything back. | ||
| */ | ||
| async function verifyWindowsSchedulerInstallAfterSettle( | ||
| options: ApplyElevatedOptions, | ||
| ): Promise<WindowsSchedulerInstallVerification | null> { | ||
| const verify = finalizeHooks?.verify ?? verifyWindowsSchedulerInstall; | ||
| let verification = verify(); | ||
| for (const delayMs of SCHEDULER_SETTLE_DELAYS_MS) { | ||
| if (!schedulerVerificationMaySettle(verification)) break; | ||
| if (!attemptStillOwned(options)) return null; | ||
| await settleDelay(delayMs); | ||
| if (!attemptStillOwned(options)) return null; | ||
| verification = verify(); | ||
| } | ||
| return verification; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether windows-elevation-spawn.test.ts covers the
# task-not-installed + native-status-unknown compound transient case.
rg -n -B3 -A15 'nativeStatus.*unknown|nativeStatusUnknown' tests/windows-elevation-spawn.test.tsRepository: lidge-jun/opencodex
Length of output: 7098
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- source symbols and implementation ---'
ast-grep outline src/service.ts --match 'schedulerVerificationMaySettle' --view expanded || true
sed -n '960,1115p' src/service.ts
printf '%s\n' '--- settle-related tests ---'
sed -n '744,955p' tests/windows-elevation-spawn.test.ts
printf '%s\n' '--- relevant type and callers ---'
rg -n -B5 -A20 'interface WindowsSchedulerInstallVerification|type WindowsSchedulerInstallVerification|schedulerVerificationMaySettle|preserveElevatedTask|nativeStatusUnknown' src/service.ts tests/windows-elevation-spawn.test.tsRepository: lidge-jun/opencodex
Length of output: 41534
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- verification probe construction ---'
sed -n '850,945p' src/service.ts
printf '%s\n' '--- exact compound-case coverage ---'
rg -n -B8 -A18 'taskInstalled:\s*false[\s\S]{0,500}nativeStatusUnknown:\s*true|nativeStatusUnknown:\s*true[\s\S]{0,500}taskInstalled:\s*false' tests/windows-elevation-spawn.test.ts || true
printf '%s\n' '--- predicate truth table ---'
python3 - <<'PY'
from itertools import product
def current(v):
if v["ok"] or v["conflict"] or not v["assetsHealthy"] or not v["nativeServiceAbsent"]:
return False
return not v["taskInstalled"] or not v["registrationHealthy"]
def proposed(v):
if v["ok"] or v["conflict"] or not v["assetsHealthy"]:
return False
proven_present = not v["nativeServiceAbsent"] and not v["nativeStatusUnknown"]
if proven_present:
return False
if not v["nativeServiceAbsent"] and v["taskInstalled"]:
return False
return not v["taskInstalled"] or not v["registrationHealthy"]
for task, absent, unknown, healthy in product((False, True), (False, True), (False, True), (False, True)):
v = {
"ok": False, "conflict": False, "assetsHealthy": True,
"taskInstalled": task, "nativeServiceAbsent": absent,
"nativeStatusUnknown": unknown, "registrationHealthy": healthy,
}
if current(v) != proposed(v):
print(v, "current=", current(v), "proposed=", proposed(v))
PYRepository: lidge-jun/opencodex
Length of output: 4551
Retry when the task is not visible and nativeStatusUnknown is true. At src/service.ts:1009-1014, !v.nativeServiceAbsent blocks settling even when v.nativeStatusUnknown is true and v.taskInstalled is false. The code then rolls back the task. Preserve the current fail-closed behavior for proven WinSW presence and for task-installed unknown status. Add a regression test for this compound case in tests/windows-elevation-spawn.test.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/service.ts` around lines 985 - 1043, Update
schedulerVerificationMaySettle to allow retries when the task is not installed
and nativeStatusUnknown is true, without retrying proven WinSW presence or an
installed task with unknown status. Preserve existing conflict and asset checks,
and add a regression test covering this compound case in the Windows elevation
spawn tests.
| # Provenance of $BunPath, chosen when the tray entry was built. Optional so an | ||
| # already-installed launcher command from an older version still starts. | ||
| [ValidateSet("", "override", "bundled", "process")][string]$BunRuntimeSource = "", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find how windows-tray.ps1 is spawned, to check whether its own
# process environment could already carry OCX_BUN_RUNTIME_SOURCE/PATH.
rg -n -B5 -A15 'windows-tray\.ps1' src/tray/windows.tsRepository: lidge-jun/opencodex
Length of output: 849
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- windows-tray.ps1 relevant sections ---'
sed -n '1,24p' src/tray/windows-tray.ps1
sed -n '80,112p' src/tray/windows-tray.ps1
printf '%s\n' '--- tray launcher and environment handling ---'
sed -n '1,180p' src/tray/windows.ts
printf '%s\n' '--- provenance references ---'
rg -n -C3 'OCX_BUN_RUNTIME_(SOURCE|PATH)|BunRuntimeSource|opencodex-tray\.vbs|windows-tray\.ps1' src structure 2>/dev/nullRepository: lidge-jun/opencodex
Length of output: 24754
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path("src/tray/windows-tray.ps1")
text = p.read_text(encoding="utf-8")
for key in ("OCX_BUN_RUNTIME_SOURCE", "OCX_BUN_RUNTIME_PATH"):
print(key, "occurrences:", text.count(key))
print("parameter default:")
for line_no, line in enumerate(text.splitlines(), 1):
if "BunRuntimeSource" in line or "EnvironmentVariables" in line:
print(f"{line_no}: {line}")
PY
printf '%s\n' '--- all Windows tray entry creation and invocation sites ---'
rg -n -C5 'sourceTrayScriptPath|windows-tray|BunRuntimeSource|OCX_BUN_RUNTIME' src --glob '!src/tray/windows-tray.ps1'Repository: lidge-jun/opencodex
Length of output: 23021
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- exact script call graph ---'
rg -n -C8 'Start-OcxCommand|BunRuntimeSource|ArgumentList|EnvironmentVariables' src/tray/windows-tray.ps1
printf '%s\n' '--- launch command construction ---'
rg -n -C12 'sourceTrayScriptPath|powershell|pwsh|opencodex-tray|launcherPath|BunRuntimeSource' src/tray/windows.ts src/tray --glob '!src/tray/windows-tray.ps1'
printf '%s\n' '--- design rule ---'
rg -n -C5 'marker|describes.*binary|OCX_BUN_RUNTIME' structure src 2>/dev/nullRepository: lidge-jun/opencodex
Length of output: 50376
🌐 Web query:
.NET ProcessStartInfo EnvironmentVariables collection default inherits current process environment Windows PowerShell
💡 Result:
In.NET, the EnvironmentVariables (or the newer Environment property) collection within ProcessStartInfo automatically inherits the environment variables of the current process upon being accessed for the first time [1][2]. Key details include: 1. Lazy Initialization: The collection is populated with the current process's environment variables only when you first access the property [2][3]. Before that first access, the underlying storage is typically null [2][3]. 2. Default Behavior: Because it is initialized with a copy of the current environment, any subsequent modifications you make to this dictionary (such as adding or removing variables) will apply only to the new child process being started, not the current process [4][5]. 3. Windows PowerShell Context: The Start-Process cmdlet in PowerShell provides a high-level wrapper around this functionality [6]. By default, Start-Process inherits environment variables from the parent process [6]. If you use the -Environment parameter in PowerShell 7.4 or later, you can specify a hash table to override or supplement these variables for the new process [6]. Note that if you use -UseNewEnvironment, PowerShell ignores the parent process's environment variables and uses only the default system-level variables (Machine and User scopes) [6]. 4. Important Limitation: You must set the UseShellExecute property to false on the ProcessStartInfo object to manually modify the environment variables [4][5][7]. If UseShellExecute is true, attempting to start the process after accessing or modifying the environment variables will result in an InvalidOperationException [4][5][7]. Additionally, certain operations, such as starting a process with -Verb RunAs in PowerShell, implicitly require UseShellExecute = true, which makes it impossible to modify the environment via these properties [8].
Citations:
- 1: https://source.dot.net/System.Diagnostics.Process/System/Diagnostics/ProcessStartInfo.cs.html
- 2: https://github.com/dotnet/runtime/blob/bbdd508a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs
- 3: Diagnostics.Process.Start: UseShellExecute=true and EnvironmentVariables gotcha dotnet/runtime#58069
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.processstartinfo.environment?view=net-9.0
- 5: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.processstartinfo.environmentvariables?view=net-9.0
- 6: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/start-process?view=powershell-7.4
- 7: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.processstartinfo.environmentvariables?view=netframework-4.8.1
- 8:
Start-Processon Windows:-Environmentis quietly ignored when-Verb RunAsis also specified, combination should be prevented. PowerShell/PowerShell#20923
🌐 Web query:
site:github.com/lidge-jun/opencodex "BunRuntimeSource" "windows-tray.ps1"
💡 Result:
The term "BunRuntimeSource" is not a recognized standard component, library, or script in the context of Windows development, PowerShell, or the Bun runtime ecosystem [1][2][3]. Based on research into technical repositories that interact with both Bun and Windows automation, the query likely refers to a misunderstanding or a specific internal identifier within the "opencodex" project (a repository utilizing Bun for cross-platform task management) [1][4][3]. In that project, automation scripts (such as install.ps1 or service lifecycle workflows) are used to handle service registration and task scheduling on Windows [4][3][5]. While these scripts utilize Bun as the runtime for the CLI, they do not include a file named "windows-tray.ps1" nor reference a "BunRuntimeSource" as a standalone entity [4][3][5]. If you are encountering a specific error message mentioning "BunRuntimeSource," it is likely a misinterpretation of a stack trace or an internal variable name from a proprietary or niche open-source codebase. You may want to check the specific documentation or issue tracker for the software you are running, as identifiers like these are often implementation-specific to a particular project's build or runtime handling logic [1][2][3].
Citations:
- 1: Windows (non-English / Bun): ocx service & ocx update abort — probeScmRegistration fails to detect ERROR_SERVICE_DOES_NOT_EXIST (1060) #216
- 2: 9eaff97
- 3: https://github.com/lidge-jun/opencodex/tree/main/scripts
- 4: https://github.com/lidge-jun/opencodex/blob/2421b4f6e19244b8c876f3bc7848eb9d1daf58d9/.github/workflows/service-lifecycle.yml
- 5: https://github.com/lidge-jun/opencodex/blob/34d4b74f07a380f016fc72e1322f2511149e28d0/.github/workflows/service-lifecycle.yml
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Windows tray command execution ---'
sed -n '130,195p' src/tray/windows.ts
sed -n '550,680p' src/tray/windows.ts
printf '%s\n' '--- tray control callers ---'
rg -n -C8 'installWindowsTray|startWindowsTray|runWindowsTrayAction|windowsTrayProcessArgs|buildWindowsTrayPowerShellCommand' src/server src/tray --glob '*.ts'
printf '%s\n' '--- marker semantics ---'
sed -n '112,160p' structure/05_gui-and-management-api.md
sed -n '52,115p' src/lib/bun-runtime.tsRepository: lidge-jun/opencodex
Length of output: 33706
Remove inherited provenance keys when $BunRuntimeSource is empty.
src/tray/windows.ts:542-546 passes process.env to the PowerShell host, and src/tray/windows-tray.ps1:98-103 copies that environment into the child. An older launcher can omit -BunRuntimeSource, allowing a stale marker for another $BunPath to reach the child. reportedBunRuntimeSource() trusts the source value without checking the path, so /api/system/memory and ocx doctor can report incorrect provenance. Add an else branch that removes both keys.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 8-8: The parameter 'BunRuntimeSource' has been declared but not used.
(PSReviewUnusedParameter)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tray/windows-tray.ps1` around lines 6 - 8, Update the environment
propagation logic in windows-tray.ps1 to add an else branch for an empty
$BunRuntimeSource that removes both inherited Bun provenance keys before
launching the child. Preserve the existing explicit-source behavior, and ensure
stale values from process.env cannot reach reportedBunRuntimeSource().
| describe("withProcessRuntimeProvenance (execPath relaunch paths)", () => { | ||
| // Under `bun test` the runner may itself BE the bundled binary, in which case | ||
| // `bundled` is the correct answer rather than `process`. Both are legitimate; | ||
| // what matters is that a real origin is always recorded. | ||
| const executingOrigin = bundledBunPath() === process.execPath ? "bundled" : "process"; | ||
|
|
||
| it("records the executable's real origin when the relaunching parent carries no marker", () => { | ||
| // `ocx ensure`, GUI start, restart, and update-relaunch all re-exec | ||
| // process.execPath. Without this they would hand the daemon no provenance at | ||
| // all, and doctor would report unknown for an origin the launcher knew. | ||
| expect(withProcessRuntimeProvenance({})[BUN_RUNTIME_SOURCE_ENV]).toBe(executingOrigin); | ||
| }); | ||
|
|
||
| it("preserves an inherited marker instead of relabeling the same runtime", () => { | ||
| // Re-execing the current runtime does not change how that runtime was obtained, | ||
| // but the claim is only carried forward when the binary it was minted for is the | ||
| // one about to run. The recorded path is what settles that — re-deriving the | ||
| // selection would demote a service installed with a shell-local override. | ||
| const overrideEnv = { | ||
| [BUN_RUNTIME_SOURCE_ENV]: "override", | ||
| [BUN_RUNTIME_PATH_ENV]: process.execPath, | ||
| }; | ||
| expect(withProcessRuntimeProvenance(overrideEnv)[BUN_RUNTIME_SOURCE_ENV]).toBe("override"); | ||
|
|
||
| // Crucially this holds with no OPENCODEX_BUN_PATH in the environment at all: an | ||
| // installed service keeps neither the shell that installed it nor its variables. | ||
| expect(overrideEnv).not.toHaveProperty("OPENCODEX_BUN_PATH"); | ||
|
|
||
| // The pair is re-stamped for the child, so the next relaunch can do the same check. | ||
| expect(withProcessRuntimeProvenance(overrideEnv)[BUN_RUNTIME_PATH_ENV]).toBe(process.execPath); | ||
| }); | ||
|
|
||
| it("drops an inherited marker that no longer describes the running binary", () => { | ||
| // Inheritance travels down a process tree, so a marker can outlive the binary it | ||
| // was minted for: something launched under a marked process but running a | ||
| // different Bun must not relaunch the daemon claiming that other binary's origin. | ||
| const staleOverride = { | ||
| [BUN_RUNTIME_SOURCE_ENV]: "override", | ||
| [BUN_RUNTIME_PATH_ENV]: join(tmp, "some-other-bun.exe"), | ||
| }; | ||
| expect(withProcessRuntimeProvenance(staleOverride)[BUN_RUNTIME_SOURCE_ENV]).not.toBe("override"); | ||
|
|
||
| // A source with no recorded path cannot be corroborated and is not carried forward. | ||
| expect(withProcessRuntimeProvenance({ [BUN_RUNTIME_SOURCE_ENV]: "override" })[BUN_RUNTIME_SOURCE_ENV]).not.toBe("override"); | ||
| }); | ||
|
|
||
| it("replaces an unrecognized inherited value rather than forwarding it", () => { | ||
| expect(withProcessRuntimeProvenance({ [BUN_RUNTIME_SOURCE_ENV]: "system" })[BUN_RUNTIME_SOURCE_ENV]).toBe(executingOrigin); | ||
| }); | ||
|
|
||
| it("leaves every other variable untouched", () => { | ||
| const result = withProcessRuntimeProvenance({ OCX_SERVICE: "1", PATH: "/usr/bin" }); | ||
| expect(result.OCX_SERVICE).toBe("1"); | ||
| expect(result.PATH).toBe("/usr/bin"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add regression coverage for symlink resolution and Windows path casing in samePath.
These tests cover marker preservation and staleness through exact-string path comparisons only. None of them exercise the two behaviors src/lib/bun-runtime.ts's samePath (Lines 116-129) specifically added: symlink resolution via realpathSync, and case-insensitive comparison on win32. The stale-marker test at Line 190 uses a non-existent path, so it only exercises the try/catch fallback to a lexical comparison, not the resolved-path branch.
Add a symlink test: create a real file in tmp, create an fs.symlinkSync alias to it at a different path, put the alias path in BUN_RUNTIME_PATH_ENV, and assert withProcessRuntimeProvenance treats process.execPath-equivalent paths as the same binary through the symlink. This is achievable cross-platform.
For the win32 casing branch, consider stubbing process.platform (e.g., Object.defineProperty(process, "platform", { value: "win32" }) with restoration in a finally) and asserting two differently-cased paths compare equal.
The PR states live Windows validation was unavailable, which makes this coverage gap more important: it is the only way to confirm the claimed fix behaves as intended before a real Windows service exercises it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/bun-runtime.test.ts` around lines 152 - 206, Add regression coverage in
the withProcessRuntimeProvenance tests for both samePath behaviors: create a
real temporary file and symlink alias with fs.symlinkSync, then verify an
inherited marker using the alias is preserved when it resolves to
process.execPath; also exercise the win32 case-insensitive branch by temporarily
stubbing process.platform, asserting differently cased equivalent paths compare
as equal, and restoring the platform value in a finally block.
Source: Path instructions
| test("ownership lost around a non-retryable failure skips rollback too", async () => { | ||
| // The settle loop never awaits for a non-retryable verdict, so this is the only | ||
| // path that reaches the pre-rollback ownership fence: a stale attempt must not | ||
| // delete a task that a newer attempt now owns. | ||
| mockParentRollbackSpawn(); | ||
| let owned = true; | ||
| let probes = 0; | ||
| setFinalizeWindowsSchedulerHooksForTests({ | ||
| elevateCreateAndRun: succeedingElevation(), | ||
| verify: () => { | ||
| probes += 1; | ||
| owned = false; | ||
| return unhealthyVerify(); | ||
| }, | ||
| settleDelay: async () => { throw new Error("must not settle a non-retryable verdict"); }, | ||
| stillOwnsAttempt: () => owned, | ||
| writeInstallState: () => { writeCount += 1; }, | ||
| }); | ||
|
|
||
| await expect(finalizeWindowsSchedulerServiceRegistration()).resolves.toEqual({ kind: "done" }); | ||
| expect(probes).toBe(1); | ||
| expect(writeCount).toBe(0); | ||
| expect(parentRollbackLaunches).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Exercise the actual rollback and final-write ownership fences.
unhealthyVerify() is retry-eligible. It has healthy assets, nativeServiceAbsent: true, and conflict: false. The plan requires retries for this state. Therefore, Line 975 does not create the non-retryable failure described by this test.
Replace this fixture with a non-retryable result, such as assetsHealthy: false or conflict: true. Then revoke ownership in verify and assert that rollback does not start.
Add a separate test where verify revokes ownership and returns okVerify(). Assert that writeInstallState is not called. The current test at lines 945-961 only covers ownership loss during a delay. It does not protect the final successful verification-to-state-write race.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/windows-elevation-spawn.test.ts` around lines 963 - 986, The test
“ownership lost around a non-retryable failure skips rollback too” must use a
genuinely non-retryable verification result, such as unhealthy assets or a
conflict, while still revoking ownership and asserting rollback is not launched.
Add a separate test for the successful path where verify returns okVerify()
after revoking ownership, and assert writeInstallState is not called to cover
the final verification-to-write ownership fence.
1 participant
Maintainer takeover of #868 and #861 (issue #848). Both were re-derived against current
devrather than cherry-picked — the audit found real gaps in each original patch, listed below.Bug A — post-create scheduler verification never settles (#868)
applyElevatedSchedulerResult()verified exactly once and rolled back immediately. After a successful elevated/create+/run, the non-elevated/query /tnand its CSV fallback can both miss the just-created task, soprobeWindowsSchedulerTask()returnsabsent,windowsSchedulerTaskInstalled()collapses that tofalse, and the evaluator reports "Task Scheduler task is not installed." A healthy registration was deleted milliseconds before Task Scheduler would have listed it.Verification now re-checks on a bounded 1.1s backoff (
[50, 150, 300, 600]), but only while the failure looks like a lagging view: the task is not visible yet, or it is visible without a fully published registration. Every other verdict keeps its meaning and spends no delay at all.Two things the original PR did not cover:
conflict.conflictonly becomes true once the task itself is visible, so while the task is still invisible beside a running WinSW the pair isconflict: falsewithnativeServiceAbsent: false. A predicate checking only!conflictwould wait for a service already proven present. There is a dedicated regression for exactly that state.ownership lost around a non-retryable failure) rather than the mid-delay case.Eight regressions in
tests/windows-elevation-spawn.test.tscover the settle path and each fail-closed class; four fail without the source change.Live Windows validation was not available in this session. The PR body's startup-protection smoke claim is covered by fault-injection contracts instead, and the plan document says so rather than implying otherwise.
Bug B — doctor repeats
OPENCODEX_BUN_PATHwhen the override is already active (#861 / #848)The remediation branch at
src/cli/doctor.tsfired onplatform === "win32" && eagerRelay.reason === "auto-known-bad"and always printed "setOPENCODEX_BUN_PATHto a runtime you trust". The payload it reads carried no runtime origin at all, so it could not tell an active override from bundled or process execution.Origin cannot be recovered after the fact: resolving it at report time answers what would this shell pick now, which is a different question from what was the service started with, and the two diverge exactly when it matters. So the selecting launcher stamps it.
Scope grew twice during review:
process.execPathrelaunches too.ocx ensure, GUI/Claude/OpenCode start,POST /api/system/restart, and the update relaunch copied the parent environment and handed the daemon nothing.withProcessRuntimeProvenance()covers those.Two design corrections came out of the audit loop:
ensureinvocation (assignment prefix insh,setlocal/endlocalincmd, save-and-restore in PowerShell) instead of exporting it. A shim wraps the realcodex, so an exported marker would be inherited by Codex and everything it spawns — a shell beneath it running a different Bun would carry provenance describing a binary it is not executing.OCX_BUN_RUNTIME_PATHbesideOCX_BUN_RUNTIME_SOURCE). An earlier version re-derived the original selection to validate an inherited claim, which demoted a correctoverridetoprocesson a service's first relaunch: an installed service keeps neither the installing shell nor itsOPENCODEX_BUN_PATH. Comparison goes throughrealpathso symlinks, junctions, and Windows case differences do not reject a valid match.Read-back allowlists the three values and never falls back to resolving locally. A service installed before the marker existed reports nothing, and doctor says the origin is unknown instead of guessing — an absent marker is an answer, not a gap to fill.
bunRevisionstays informational.src/lib/bun-stream-caps.tsandsrc/server/responses/core.tsare absent from every commit here: the conservativeauto-known-badresult for canaries and the eager-relay capability policy are untouched, as the issue directed.Verification
bun run typecheck— exit 0bun run test— 6988 pass / 8 skip / 0 fail across 480 files on this branch rebased ontodevstructure/05_gui-and-management-api.mddocuments the provenance trust rule and the backward-compatibility contractCloses #848.
Summary by CodeRabbit
Bug Fixes
Documentation