Repository navigation
fix(desktop): preserve Windows opencli launcher paths - #6121
Conversation
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed: full first review of 99dbf56 against main (one commit; touches apps/desktop/src/main/opencli-chrome.ts and its test). No prior findings. No protocol files are touched, so there is no epoch impact.
Verdict: no P0-P2 findings. I think the fix is correct.
Why it is correct:
- cmd.exe decodes each batch line with the console code page that is active when it reads that line. The lines before
chcp 65001are pure ASCII, so the byte offsets do not move. After the switch, the UTF-8 path line is decoded correctly. Previously the launcher misread non-ANSI paths on CP936 systems. - Doubling
%to%%keeps%NAME%literal in the percent-expansion phase.setlocal DisableDelayedExpansionkeeps!NAME!literal even when the caller enables delayed expansion (/v:onor the registry). Characters like&,^,|,<and>were already literal inside the double quotes, and a Windows path cannot contain". So there is no new injection surface.executableandentryareprocess.execPathand a bundled dist path in any case. - The POSIX launcher is byte-for-byte unchanged.
chcpoutput goes to>nul, so nothing reaches the Native Messaging stdout channel.
Tests: I built the PR on Linux with #6122 and #6123 merged in. opencli-chrome.test.js gave 4 passed and 1 skipped. With opencli-chrome.ts reverted to main, only the exact-string assertion in "Windows launchers set Node mode before starting the entry" fails on Linux. The behavioural regression is Windows-only and skipped here.
Findings:
- P3:
chcpis not scoped bysetlocal. It changes the code page of the console the launcher is attached to, and the change outlives the launcher. Chrome and Maka's own spawns get a private hidden console, so this is harmless there. If an MCP client runsopencli-mcp.cmdfrom the user's own terminal, that terminal stays on 65001 afterwards. One option is to save the old code page and restore it after the child exits (this keeps the exit code). Another is to record this as accepted. See the inline note. - P3: The new Windows regression test (
skip: process.platform !== 'win32') is not run by any CI lane.apps/desktopis outsidewindows-recovery.yml, andwindows-baseline.ymldoes not run desktop suites. On CI the fix is covered only by the string assertion. The PR description reports local Windows runs. Thewin32path-filter rule does not apply, because the rule covers onlypackages/files in the recovery suites' closure.scripts/windows-package-source-closure.test.mjspasses on the merged tree.
Overlap: no files in common with #6122 or #6123. All three merge onto main together without conflicts.
CI and mergeability: GitHub reports MERGEABLE, BLOCKED (review required). The test and label jobs are still queued, so there is no CI result on this head yet.
Restore the caller's console code page without changing child exit status or protocol output. Run the native launcher regression in Windows recovery CI. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 99dbf56af196b37cdfb2f11f90cf1edbccbb9dea..dd42e95ca20324378f6d4a1809715acdc6c09938, one commit that restores the caller's console code page after the launcher runs, keeps the child's exit status, and runs opencli-chrome.test on the Windows recovery lane (both launcher files added to the path filter).
The previous P3s are fixed: the launcher saves the page from chcp, switches to 65001, and restores the saved page before exit /b with the child's status; the test now calls the launcher from a caller batch at code pages 936 and 65001 and asserts both the caller's page and exit status 23 afterwards. The Windows recovery run on this head passed, including the new step "Verify Windows opencli launchers".
P3 (non-blocking): the saved page is parsed with for /f "tokens=2 delims=:" from the localized chcp output. Some Windows display languages print a trailing period (for example German, Aktive Codepage: 850.), which makes the restore command chcp 850. fail, leaving the console at 65001 and writing an error to stderr. Stripping non-digits (or reading the page from the registry) would cover those locales. This only affects the interactive-terminal case noted before; hidden consoles started by Chrome or Maka are unaffected. No new P0–P2 findings.
No protocol files touched (epoch unaffected). The PR is MERGEABLE; the required test job (CI) is still queued, so this is not a claim of green CI or an approval.
Treat the trailing period in localized chcp reports as a delimiter. Exercise the generated batch parser with German query output while checking real Windows code-page restoration, protocol output and child exit status. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: dd42e95c..61e53211, one commit that adds . to the for /f delimiters used to capture the caller's code page, plus a Windows-only regression. I found no P0–P2 issue.
- The fix is right. Some localized
chcpresponses end the number with a period (for example GermanAktive Codepage: 850.). Withdelims=:the saved value was850., sochcp %MAKA_OPENCLI_CODE_PAGE%failed on restore and the caller was left on 65001. Withdelims=:., token 2 is850, andchcpaccepts the leading space. English, French (Page de codes active : 850), CJK and Russian responses parse the same as before. - The regression tests the real path. It replaces only the
chcpquery with a localizedecho. cmd.exe still runs the generated parser, really switches to 65001, and restores the page the caller set (850, 936 and 65001), while the child's stdout and exit status (23) pass through unchanged. The test file is already on the Windows recovery lane's path filter from the previous commit.
P3 (optional): parse the last token instead of token 2. Token 2 assumes the localized label before the number contains no : or .. I don't know of a Windows locale where it does. If one exists, the saved value would be part of the label and the restore would fail as before. Taking the last numeric token (for example, looping for %%t in (...) do set ..., so the last value wins) would remove that assumption. This is not a regression from this commit.
CI test is still running on this head.
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.Incremental review:
61e53211..ebf4a480, one merge of main atd14cec57to pick up the CI fixes from #6129. The merge applied without conflicts:git show --remerge-diffis empty, so it adds no author change. My earlier conclusions still hold: no P0–P2 issue. CItestnow passes on this head, and the PR is mergeable.
|
Thanks for the contribution and for addressing the review feedback. Merging now. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: no blocking findings in the automated review of this head, and CI is green.
Summary
Fix Windows opencli launchers failing with Chinese paths on CP936 systems or literal
%NAME%in installation paths.Generate UTF-8 batch files with quiet code-page selection, escaped percent signs and delayed expansion disabled. Preserve MCP/Chrome launcher modes, output and exit codes.
Verification
%NAME%,!NAME!,&, both launcher modes and exit status.AI use
Tool(s) and scope: OpenAI Codex assisted with investigation, implementation, regression tests, review and verification. The commit includes
Generated-by: Codex.Checklist
Does this PR entail a change in behavior?