fix(tray): keep Windows Run command under 260 chars - #771
Conversation
📝 WalkthroughWalkthroughThe Windows tray installer now writes an owned VBS launcher, registers a short ChangesWindows tray autostart
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsSignIn
participant HKCURun
participant WScript
participant PowerShellTrayHost
WindowsSignIn->>HKCURun: Execute registered short command
HKCURun->>WScript: Launch opencodex-tray.vbs
WScript->>PowerShellTrayHost: Run the PowerShell tray command invisibly
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/tray/windows.ts`:
- Around line 157-159: Update quoteVbsPath to stop replacing backslashes and
only escape double quotes before values are passed to WScript.Shell.Run,
preserving UNC and other Windows paths unchanged.
- Around line 531-533: Update the launcher ownership guard around the existing
existsSync check to reject an existing launcher whenever persisted state is
absent or state.launcherPath is missing; only allow it when the stored launcher
path resolves to the current launcherPath. Preserve the existing error behavior
and message for unowned launchers.
🪄 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: c09c0da2-6b49-4e94-82bc-1bacf2119939
📒 Files selected for processing (3)
src/tray/windows.tstests/windows-tray-run-limit.test.tstests/windows-tray.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52920d3189
ℹ️ 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".
|
[shipping-github] Merge-ready
|
Escape only quotes in VBS paths, refuse overwriting unowned launchers, and write UTF-16LE tray scripts so WSH reads non-ASCII paths correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/windows-tray.test.ts`:
- Around line 70-75: Update the test around buildWindowsTrayLauncherScript to
verify the actual persisted launcher bytes produced by the Windows tray
installation flow, rather than re-encoding the in-memory script with the
production expression. Assert both the UTF-16LE byte order and preservation of
the non-ASCII path from the installed output; alternatively, extract and test a
shared production encoding helper.
🪄 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: 214253a9-5ccf-436e-8895-981c2fcaa2be
📒 Files selected for processing (2)
src/tray/windows.tstests/windows-tray.test.ts
…oundary Windows CI died with `panic(...): Internal assertion failure` and no `(fail)` line — four runs across `dev`, #771, #780 and #781. The crash header names the shape: `workers_spawned(9) workers_terminated(8)`, one worker never reclaimed, and it lands exactly at the `api-storage-policy` -> `api-storage` file boundary. That suite is the only one that spawns policy workers. `Worker.terminate()` returns void and does not wait for the thread to go away. Under `bun test --isolate` the harness reclaims a file's realm at the boundary, so a worker still exiting at that instant trips a Bun-internal assertion and takes the whole run down. macOS and Linux tolerate the same race, which is why it reads as a Windows-only flake. `worker-lifecycle.ts` tracks live workers and terminates them through the `close` event, so callers can await the thread actually being gone rather than a timer they hope is long enough; a 5s cap keeps a wedged worker from hanging teardown. Both storage jobs settle their run promise after that await, and the policy suite's `afterEach` now drains. devlog `_plan/260730_remote_issue_merge_round/150` reproduced this panic twice against an unchanged tree, confirmed the same tree passed in PR context, and left this defence as the follow-up if it recurred. It recurred. Evidence: 6190 pass / 0 fail across 445 files, typecheck clean, privacy scan green. The regression test asserts a worker was really spawned before checking it is gone, so it cannot pass vacuously.
Verdict: approve-comment
Ready for maintainer merge when convenient. |
|
Landed on The VBS launcher approach is the right shape: the long PowerShell invocation lives in an owned launcher file and the Run value becomes Two notes on how it got here. The earlier conflict is gone. This PR was blocked mid-round by a conflict in The ablation initially fooled me. Disabling the 260-character guard failed nothing, which looked like a vacuous test. It is not: that guard is a startup assertion, not the fix. Substituting the old PowerShell command back into One gap worth naming: nothing invokes Closing here since the work is staged; the branch merges into |
Summary
Fixes #696
Test plan
Security
Summary by CodeRabbit
New Features
Bug Fixes
Tests