fix(windows): classify localized scheduler denial (#591) - #698
Conversation
|
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 (5)
📝 WalkthroughWalkthroughWindows Task Scheduler elevation detection now uses a locale-independent token probe. Access-denied recovery is restricted to the owned ChangesWindows elevation and Task Scheduler recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant schtasks.exe
participant isWindowsSchtasksAccessDeniedError
participant isCurrentWindowsProcessElevated
participant toWindowsSchtasksError
schtasks.exe->>isWindowsSchtasksAccessDeniedError: Return scheduler status and stderr
isWindowsSchtasksAccessDeniedError->>isCurrentWindowsProcessElevated: Check current process elevation
isCurrentWindowsProcessElevated-->>isWindowsSchtasksAccessDeniedError: Return elevated, non-elevated, or unknown
isWindowsSchtasksAccessDeniedError->>toWindowsSchtasksError: Classify as access-denied or other
Possibly related issues
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: 4
🤖 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 `@docs-site/src/content/docs/reference/cli.md`:
- Around line 442-446: Qualify the direct non-elevated Windows install guidance
in docs-site/src/content/docs/reference/cli.md:442-446 so UAC recovery and
language-independent guidance apply only when the owned opencodex-proxy command
and a confirmed non-elevated token are detected; otherwise state that the
original scheduler error is retained. Apply the equivalent translated
clarification in docs-site/src/content/docs/ja/reference/cli.md:363-367,
docs-site/src/content/docs/ko/reference/cli.md:382-386,
docs-site/src/content/docs/ru/reference/cli.md:404-408, and
docs-site/src/content/docs/zh-cn/reference/cli.md:366-369.
In `@src/lib/windows-elevation.ts`:
- Around line 322-326: Update isWindowsSchtasksAccessDeniedError so the
recognized-text branch also requires isOwnedSchedulerCreate(args) before
returning true; retain the scheduler-exit-status and elevation checks for owned
tasks. Preserve generic access-denied text for non-retry user-facing formatting,
and add a regression test covering a foreign /create command targeting
someone-elses-task with “Access is denied.”.
- Around line 49-54: Update the IsUserAnAdmin declaration and wrapper in the
Windows elevation binding: change its FFI return type from "bool" to "i32", and
have isUserAnAdminFn return whether lib.symbols.IsUserAnAdmin() is nonzero.
In `@tests/windows-elevation.test.ts`:
- Around line 107-139: Extend the locale-independent fallback test around
toWindowsSchtasksError with a confirmed-elevated probe case:
setWindowsElevationProbeForTests(() => true), invoke the owned /create
opencodex-proxy command, and assert reason is "other". Reset the probe to null
in a finally block, matching the existing false and null cases.
🪄 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: 64e18d80-28e2-4207-9fd7-44155c0706cb
📒 Files selected for processing (8)
docs-site/src/content/docs/ja/reference/cli.mddocs-site/src/content/docs/ko/reference/cli.mddocs-site/src/content/docs/reference/cli.mddocs-site/src/content/docs/ru/reference/cli.mddocs-site/src/content/docs/zh-cn/reference/cli.mdsrc/lib/windows-elevation.tsstructure/05_gui-and-management-api.mdtests/windows-elevation.test.ts
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 `@docs-site/src/content/docs/reference/cli.md`:
- Around line 442-447: Clarify the documentation so fail-closed behavior for an
unavailable elevation probe applies only to the status-1, unrecognized-text
fallback; preserve the existing localized access-denied text-detection path.
Make this correction in docs-site/src/content/docs/reference/cli.md lines
442-447, docs-site/src/content/docs/ja/reference/cli.md lines 363-368,
docs-site/src/content/docs/ko/reference/cli.md lines 382-386,
docs-site/src/content/docs/ru/reference/cli.md lines 404-409, and
docs-site/src/content/docs/zh-cn/reference/cli.md lines 366-370.
🪄 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: 4b71431c-b352-41a6-9420-b48084264bdb
📒 Files selected for processing (7)
docs-site/src/content/docs/ja/reference/cli.mddocs-site/src/content/docs/ko/reference/cli.mddocs-site/src/content/docs/reference/cli.mddocs-site/src/content/docs/ru/reference/cli.mddocs-site/src/content/docs/zh-cn/reference/cli.mdsrc/lib/windows-elevation.tstests/windows-elevation.test.ts
|
All automated checks are now green, including the 10m45s I am not self-approving or self-merging this UAC-boundary change. After approval, the merger must also satisfy the documented |
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 `@docs-site/src/content/docs/reference/cli.md`:
- Around line 442-448: Update the UAC-recovery documentation in
docs-site/src/content/docs/reference/cli.md lines 442-448 to state that the
fixed task-create command requires the exact /create command, expected XML path,
and /f checks, in addition to the owned task name, exit status 1, and confirmed
non-elevated token. Translate and apply the same complete predicate in
docs-site/src/content/docs/ja/reference/cli.md lines 363-370,
docs-site/src/content/docs/ko/reference/cli.md lines 382-388,
docs-site/src/content/docs/ru/reference/cli.md lines 404-411, and
docs-site/src/content/docs/zh-cn/reference/cli.md lines 366-371, keeping all
locale guidance aligned with the CLI behavior.
🪄 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: 55f0e29c-d7a7-4cc7-8db8-efe69e7ca937
📒 Files selected for processing (5)
docs-site/src/content/docs/ja/reference/cli.mddocs-site/src/content/docs/ko/reference/cli.mddocs-site/src/content/docs/reference/cli.mddocs-site/src/content/docs/ru/reference/cli.mddocs-site/src/content/docs/zh-cn/reference/cli.md
|
CI update: the docs clarification run had one unrelated Windows timeout in the existing I reran the failed Actions run without changing code. The Windows full runtime |
|
Created the required |
…ion hardening, Windows scheduler fixes Promotes dev to main for the v2.7.43 quick-fix release: - fix(gui): renew loopback dashboard sessions silently instead of prompting - PR #705 + follow-up: Claude effort restoration boundary hardening - PRs #694/#698/#699 and Windows scheduler compare/denial fixes
Summary
schtasks /create /tn opencodex-proxy /xml ... /fstatus-1 failure as access denied when the effective Windows token is known to be non-elevatedAddresses #591.
Root cause
On a Chinese Windows installation,
schtasks.exereturned localized access-denied output in the system OEM code page. Bun decoded the captured bytes as UTF-8 mojibake, so the English/German text matcher classified the structured scheduler error asreason: "other". The CLI lost actionable guidance and the dashboard did not receiveOCX_ERROR_CODE=WINDOWS_SCHTASKS_CREATE_ACCESS_DENIED, so its existing trusted UAC transaction was never selected.The reporter's later diagnostics also show that the elevated installation itself succeeded. Their remaining Startup Safety warning followed
ocx restore, which intentionally returns Codex to native/external routing;ocx restore backis the correct way to re-enable OpenCodex routing.Security boundary
This does not treat arbitrary exit code 1 as permission denial. The fallback requires all of:
/createoperation,opencodex-proxy,/f,Unknown token state remains
reason: "other". The elevated transaction continues to resolve trusted System32 executables, accept only the fixed scheduler protocol, and fail closed on unexpected outcomes.Microsoft documents
IsUserAnAdminas a wrapper overCheckTokenMembership; the latter only reports an enabled Administrator SID, so a UAC-filtered deny-only SID does not grant this signal:Validation
bun x tsc --noEmitbun test --isolate tests— 5901 passed, 1 Windows-only runtime test skipped, 0 failedbun run privacy:scancd docs-site && bun install --frozen-lockfile && bun run build— 146 pages builtdev2-go transition
devhas nogo/tree, so this PR cannot carry the native Go scheduler counterpart.dev2-godoes havego/internal/service/install.go, whose scheduler path currently returns raw localizedschtasks.exefailures and has no equivalent UAC marker/transaction. PerMAINTAINERS.md, merging this PR intodevis not the end of the integration task: the merger must assess and port the behavior todev2-goin the same session, or open aneeds-go-porttracking issue naming this source commit (cdc16e5a) before closing the merge work.Summary by CodeRabbit
Bug Fixes
ocx service installerror handling for localized environments by recognizing “access denied” only for the app’s own fixed Task Scheduler create attempt and confirming non-elevated token state.Documentation
Tests