Skip to content

fix(windows): stop console popups from proxy-internal PowerShell lookups (#1236) - #1347

Open
wade19990814-hue wants to merge 2 commits into
lidge-jun:devfrom
wade19990814-hue:fix/windows-popup-hidden-spawn
Open

fix(windows): stop console popups from proxy-internal PowerShell lookups (#1236)#1347
wade19990814-hue wants to merge 2 commits into
lidge-jun:devfrom
wade19990814-hue:fix/windows-popup-hidden-spawn

Conversation

@wade19990814-hue

@wade19990814-hue wade19990814-hue commented Aug 9, 2026

Copy link
Copy Markdown

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 popup reported in #1236: 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() in src/codex/user-identity.ts (both the SID and the LocalAppData lookups) spawned bare powershell.exe without windowsHide — Windows allocates a fresh visible console for each console-subsystem child.

Changes:

  • src/codex/user-identity.ts — the identity lookups now spawn hidden (windowsHide: true plus -WindowStyle Hidden), under a bounded 8s timeout (a hung PowerShell fails the lookup, which callers refuse, instead of wedging startup/config writes), and from resolveTrustedWindowsPowerShellExe() (System32 via GetSystemDirectoryW, never PATH). Test-only readbacks follow the existing windowsPrincipalPowerShellCommandForTests seam pattern.
  • src/codex/app-server-processes.ts — the three enumeration/start-time PowerShell sites (already windowsHide + timeout) now resolve the executable through resolveTrustedWindowsPowerShellExe() instead of bare PATH powershell.exe.
  • windows-user-principal.ts and native-profile-processes.ts are already hardened on dev and are untouched.

Windows regression coverage: tests/windows-popup-fix.test.ts asserts 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.ts on a Windows host — 117/119 pass. The two codex-user-identity.test.ts failures reproduce identically on pristine origin/dev HEAD (verified in a clean worktree); they are host-environment artifacts of this machine's registry (%USERPROFILE% in User Shell Folders) and not regressions from this PR.
  • bun run privacy:scan — pass.
  • Popup behavior verified experimentally beforehand: a child spawned from a console-less parent with windowsHide: true gets no console allocated; without the flag it does.
  • Real-machine end-to-end check on the reporter's Windows desktop: built this branch from source (Vite dashboard build + 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 doctor green after the swap.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (No doc change needed: this removes unintended popup behavior; no configuration or documented contract changes.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (Executable resolution strictly tightens: trusted System32 via GetSystemDirectoryW instead of PATH.)

Closes #1236

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

  • Bug Fixes

    • Improved Windows process and identity detection by using trusted PowerShell execution.
    • Prevented unwanted PowerShell console windows from appearing.
    • Added time limits and safer fallback behavior when Windows identity lookups fail or time out.
  • Tests

    • Added regression coverage for hidden PowerShell execution, identity detection, process start-time lookup, and cross-platform fallback behavior.

…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.
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 9, 2026
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f699c48b-0161-4c29-b6e8-d6e67255da2f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Windows 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.

Changes

Windows PowerShell hardening

Layer / File(s) Summary
Trusted process lookup execution
src/codex/app-server-processes.ts
The import at line 13 and lookup paths at lines 349–352, 463, and 519 now use resolveTrustedWindowsPowerShellExe() instead of PATH-based powershell.exe. Hidden-window execution remains enabled.
Hidden bounded identity lookup
src/codex/user-identity.ts, tests/windows-popup-fix.test.ts
Lines 20–114 centralize trusted command construction, hidden non-interactive spawn options, piped output, and an 8-second timeout. Resolution failures and timeouts return identity refusals. Lines 1–78 add regression coverage for these settings, identity resolution, and cross-platform process-time fallback.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

Possibly related PRs

  • lidge-jun/opencodex#736 — Both PRs modify Windows identity verification and PowerShell process handling.
  • lidge-jun/opencodex#1279 — Both PRs modify the same Windows identity and process-lookup paths with trusted PowerShell execution safeguards.

Suggested reviewers: wibias, ingwannu, lidge-jun

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main Windows fix: preventing console popups from internal PowerShell lookups.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ 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.

4/4 boxes ticked.

This pull request has been marked Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently. If no CodeRabbit review appears, comment @coderabbitai review to request one.
Maintainers notified: @lidge-jun @Ingwannu @Wibias

@github-actions
github-actions Bot marked this pull request as draft August 9, 2026 08:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 025c379 and 57c0a2b.

📒 Files selected for processing (3)
  • src/codex/app-server-processes.ts
  • src/codex/user-identity.ts
  • tests/windows-popup-fix.test.ts

Comment thread tests/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.
@wade19990814-hue
wade19990814-hue marked this pull request as ready for review August 9, 2026 09:12
@wade19990814-hue

Copy link
Copy Markdown
Author

@lidge-jun A quick status update on the focused resubmission you asked for in #1279.

Real-machine verification. The reporter built this exact branch (build:gui + npm pack) and installed it over the stock 2.11.0 global install on the affected Windows desktop — the machine where a visible console window popped on every startup, config change, and shutdown. After the swap, all three scenarios run with no console window appearing, and ocx doctor is green. So the bounded fix does close #1236 in practice, not just in tests.

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 dev commit, and the findings above are resolved.

One request: the Cross-platform CI workflow for this fork PR is currently pending approval (action_required), so the hosted matrix has not run yet. Approving it would let the CI gate verify the head; I will flag anything it surfaces immediately.

@github-actions
github-actions Bot marked this pull request as draft August 9, 2026 09:12
@github-actions
github-actions Bot marked this pull request as ready for review August 9, 2026 09:14
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

@github-actions[bot] The PR readiness gate is complete. I will review the current PR head.

⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant