Skip to content

fix(service): keep % expansions out of parens in Windows scheduler wrapper (#6290) - #6406

Closed
devin-ai-integration[bot] wants to merge 2 commits into
devfrom
devin/1790865681-windows-wrapper-parens
Closed

devin-ai-integration[bot] wants to merge 2 commits into
devfrom
devin/1790865681-windows-wrapper-parens

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes the Windows Task Scheduler service wrapper dying at parse time on systems where a %-expanded value contains ) — most visibly ko-KR/ja-JP locales, where %DATE% expands like 2026-09-30(수) (#6290, same defect class as #5093).

cmd.exe percent-expands variables while parsing a ( ... ) block, before the line's condition is ever tested. The wrapper's if exist (...), if not exist (...), and for /f ... in (...) do (...) blocks all embedded %OCX_API_TOKEN_FILE%, %OCX_PKG_DIR%, %OCX_CLI%, and timestamped %DATE% %TIME% echoes inside parentheses. A ) inside any of those values closed the block early, the parse error aborted the whole batch before the bun launch line, and the proxy silently never started on logon — Task Scheduler's Last Run Result stayed 0 so RestartOnFailure never engaged.

buildWindowsServiceScript now keeps every %VAR% expansion out of parentheses:

  • Token-file read and the four missing-install checks: single-line if/goto, no blocks.
  • OCX_PKG_DIR resolved at generation time (win32.dirname^3 of cli) instead of for %%I in ("%OCX_CLI%\..\..\..") — which both put a % inside parens and could not survive a ) in the install path. Still defined when the package tree is gone, which is exactly the restore scenario.
  • call :restore_backup unchanged; inside it, pushd "%OCX_PKG_DIR%\.." first so the for /f in-clause carries only a literal .ocx-backup-* glob, and the per-backup work is a called :try_restore subroutine instead of a nested parenthesized body.
  • New :missing_bun/:missing_cli labels hold the installation is incomplete log + exit /b 3 paths.
  • The launch/stay-out contract is byte-identical: child exit 42 → :stopped/endlocal/exit /b 0; anything else → log + ping -n 6 5s cooldown → goto loop.

matchesGeneratedStandaloneControlFlow also recognizes the new single-line token read, preserving standalone-wrapper ownership detection.

ocx service repair already rewrites these assets unconditionally on the scheduler backend, so existing installs pick up the fixed wrapper on repair/update.

Verification

  • bun test tests/windows/windows-service-wrappers.test.ts — 17 pass, incl. two new tests: a scan asserting no % inside any (...) region of both generated variants, and a win32 test that runs the generated wrapper under real cmd.exe with a UTF-8 set "DATE=2026-09-30(수)" shadow (expects clean exit 3 + the installation is incomplete log line, not a parse abort).
  • bun test tests/service/service.test.ts tests/service/standalone-service.test.ts — 209 pass, 3 platform skips.
  • bun run typecheck, bun run privacy:scan, bun run structure:check — all pass.
  • Live cmd.exe A/B on this Windows box, both generated wrappers under the simulated ko-KR %DATE%:
  • Not run: full bun run test suite and GUI/Rust gates (changed surface is the service wrapper, service-manager probe, and tests; no gui/ or desktop/ changes, so no screenshots).

CI follow-up

  • Updated the standalone probe token boundary; bun test tests/codex-service-manager-probe.test.ts passed 89 tests and typecheck passed.
  • Latest CI: 27 passed, 2 failed, 8 skipped. Windows Task Scheduler integration and all four main test shards passed.
  • Remaining failure: macos 1/2, owner-registry-acl.test.ts (registry publication requires the owner-only Windows directory ACL) returns an empty homes list. The aggregate ci gate fails with it. The corresponding 6 tests pass locally on Windows. A macOS path/no-follow issue is a hypothesis, not a confirmed root cause; no clean-base reproduction or same-revision successful retry establishes flakiness. This remains unresolved, and the PR is not fully green.

Checklist

Link to Devin session: https://app.devin.ai/sessions/641ad09cbb8f48b3a44bc2c65cd6d82f
Open in Devin Desktop: https://app.devin.ai/desktop/session/641ad09cbb8f48b3a44bc2c65cd6d82f?variant=devin

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

cmd percent-expands variables while PARSING a ( ... ) block, before the
line's condition is tested. A value containing ')' — a ko-KR/ja-JP %DATE%
like "2026-09-30(수)", or an install path under "Program Files (x86)" —
closed the block early and the parse error aborted the whole wrapper
before the bun launch line, so the proxy silently never started on logon
(Last Run Result stayed 0, RestartOnFailure never engaged).

- token read, missing-install checks: single-line if/goto, no blocks
- OCX_PKG_DIR resolved at generation time (win32.dirname^3 of cli)
- restore_backup: pushd into the package parent so the for /f in-clause
  carries a literal glob; per-backup work moved to a :try_restore call
- exit-code loop contract unchanged: 42 -> :stopped, else 5s cooldown

Verified on real cmd.exe with a UTF-8 ko-KR-style %DATE%: old wrapper
aborted with 'was unexpected at this time'; new wrapper restores the
newest .ocx-backup-*, logs 'installation is incomplete' + exit 3 when
unrecoverable, and runs the launch/restart/stay-out loop to exit 0.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: db5051b6-8b0f-47c3-99e3-dbf0bce55b09

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
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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 added the bug Something isn't working label Oct 1, 2026
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

…e token read

The flow comparator indexed the prefix-set boundary by the old
multi-line 'if exist "%OCX_API_TOKEN_FILE%" (' opener; the #6290
rewrite emits that check as one line, so expectedBoundary was -1 and
every generated standalone wrapper failed the chain walk.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ⬜ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions
github-actions Bot marked this pull request as draft October 1, 2026 16:11
@lidge-jun

Copy link
Copy Markdown
Owner

Closing as superseded by #6391 (8a3a7762fe), which fixed #6290 in the same wrapper.

An independent audit and the plan architect both checked src/service/windows-taskxml.ts on dev. Every %VAR% expansion still inside a parenthesized block is inside double quotes, " is stripped from values before they are written, delayed expansion is off, and every [%DATE% %TIME%] echo sits at a top-level label. A ) in a locale date or an install path therefore cannot close a block early. tests/windows/windows-service-wrappers.test.ts exercises parenthesized dates and a package (test)! path, and it passed on real Windows in the union run 36938434654.

Two parts of this PR should not land as written. The bracket-scan test counts parentheses inside quotes, so it fails on dev's correct wrapper. The tokenBlock change in src/service-manager-probe.ts would stop recognizing wrappers that dev generates. The ideas that remain (computing OCX_PKG_DIR in TypeScript, and a pushd-based backup listing) are a possible follow-up, not a live defect.

@lidge-jun lidge-jun closed this Oct 2, 2026
@lidge-jun
lidge-jun deleted the devin/1790865681-windows-wrapper-parens branch October 3, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant