fix(windows): stop console popups from proxy-internal PowerShell lookups (#1278) - #1347
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughWindows process and identity PowerShell lookups now use a trusted executable resolver. Identity lookups use hidden, non-interactive, piped, time-limited child processes. Regression tests cover command construction, spawn settings, identity resolution, and process-time fallback behavior. ChangesWindows PowerShell hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-popup-fix.test.ts`:
- Around line 44-50: Update the timeout assertion in the “spawn options are
hidden and bounded” test to require exactly 8 seconds, replacing the generic
finite/positive checks while preserving the existing windowsHide and stdin
assertions.
🪄 Autofix
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: 77ce49e9-c0cf-44d0-9251-64b34f189165
📒 Files selected for processing (3)
src/codex/app-server-processes.tssrc/codex/user-identity.tstests/windows-popup-fix.test.ts
CodeRabbit finding on lidge-jun#1347: a looser positivity assertion would let a silent re-tune of the bounded lookup budget through the regression test.
|
@lidge-jun A quick status update on the focused resubmission you asked for in #1279. Real-machine verification. The reporter built this exact branch ( Review findings. CodeRabbit's one actionable finding (assert the exact 8s lookup budget instead of any positive timeout) is fixed in 716b7fe. That was the only finding; the walkthrough otherwise matches the intent. Checklist. All four readiness boxes are ticked against the current head: local testing is green (typecheck, the seven targeted Windows/identity/process test files, privacy scan), the branch sits exactly on the latest One request: the Cross-platform CI workflow for this fork PR is currently pending approval ( |
|
|
Wibias
changed the title
…ups (lidge-jun#1236) The desktop proxy parent runs without a console, so every console-subsystem child spawned without CREATE_NO_WINDOW gets a fresh visible console window. user-identity's SID and LocalAppData lookups spawned powershell.exe with no windowsHide, which surfaced as popups at startup, on config writes, and on shutdown. Focused fix per the lidge-jun#1279 review: harden the existing identity and process-lookup spawn sites only — no enumeration rewrite, no POSIX changes. - user-identity: spawn the identity lookups hidden (windowsHide plus -WindowStyle Hidden), under an 8s bounded timeout, and from the trusted System32 PowerShell (never PATH). A hung child now fails the lookup instead of wedging startup. - app-server-processes: resolve the three enumeration/start-time PowerShell sites through resolveTrustedWindowsPowerShellExe(); windowsHide and timeouts were already in place there. - windows-user-principal and native-profile-processes were already hardened on dev and are untouched. Adds tests/windows-popup-fix.test.ts regression coverage for the hidden, trusted, bounded spawn shape plus a real-token check on Windows hosts.
CodeRabbit finding on lidge-jun#1347: a looser positivity assertion would let a silent re-tune of the bounded lookup budget through the regression test.
3629c40 to
6d49244
Compare
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Merge check complete on exact head |
2 participants
Summary
Focused resubmission requested in the #1279 review: hidden execution, trusted System32 resolution, and bounded timeouts applied to the existing identity and process-lookup spawn sites — preserving the current PowerShell enumeration and all POSIX start-time behavior. No enumeration rewrite, no WMIC, no FFI profile resolution; those stay separate if wanted later.
Fixes the transient proxy-internal PowerShell popup reported in #1278: the installed Windows build shows visible console windows at startup, on config changes, and at shutdown. Root cause: the proxy parent runs without a console, and
powershellValue()insrc/codex/user-identity.ts(both the SID and the LocalAppData lookups) spawned barepowershell.exewithoutwindowsHide— Windows allocates a fresh visible console for each console-subsystem child.Changes:
src/codex/user-identity.ts— the identity lookups now spawn hidden (windowsHide: trueplus-WindowStyle Hidden), under a bounded 8s timeout (a hung PowerShell fails the lookup, which callers refuse, instead of wedging startup/config writes), and fromresolveTrustedWindowsPowerShellExe()(System32 viaGetSystemDirectoryW, never PATH). Test-only readbacks follow the existingwindowsPrincipalPowerShellCommandForTestsseam pattern.src/codex/app-server-processes.ts— the three enumeration/start-time PowerShell sites (alreadywindowsHide+ timeout) now resolve the executable throughresolveTrustedWindowsPowerShellExe()instead of bare PATHpowershell.exe.windows-user-principal.tsandnative-profile-processes.tsare already hardened ondevand are untouched.Windows regression coverage:
tests/windows-popup-fix.test.tsasserts the hidden/trusted/bounded spawn shape on every host, plus real-token and real batch-start-time checks on Windows.Verification
bun x tsc --noEmit(strict) — clean.bun test tests/windows-popup-fix.test.ts tests/codex-user-identity.test.ts tests/codex-app-server-processes.test.ts tests/windows-user-principal.test.ts tests/windows-elevation.test.ts tests/windows-elevation-spawn.test.ts tests/native-profile-processes.test.tson a Windows host — 117/119 pass. The twocodex-user-identity.test.tsfailures reproduce identically on pristineorigin/devHEAD (verified in a clean worktree); they are host-environment artifacts of this machine's registry (%USERPROFILE%inUser Shell Folders) and not regressions from this PR.bun run privacy:scan— pass.windowsHide: truegets no console allocated; without the flag it does.npm pack), installed it over the existing 2.11.0 global install, then exercised startup, config change, and shutdown. No PowerShell/console window appeared in any of the three, where the stock 2.11.0 build popped one every time.ocx doctorgreen after the swap.Checklist
GetSystemDirectoryWinstead of PATH.)Fixes #1278
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests