docs: add provider selection guide and FAQ (#2300)#2305
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Windows batch launcher (codewhale.bat) that prefers Windows Terminal when executing the application. The review feedback highlights several critical issues: the environment variable NO_ANIMATIONS is set too late in the batch script, using cmd /k keeps the terminal window open unnecessarily, and guarding the generation with isWindows prevents packaging on non-Windows CI runners. Additionally, adding codewhale.bat to ASSET_MATRIX is redundant and potentially problematic, as the file is already manually generated and added to the manifest.
| if (isWindows) { | ||
| const batContent = [ | ||
| "@echo off", | ||
| "where wt >nul 2>nul", | ||
| 'if "%ERRORLEVEL%"=="0" (', | ||
| ' wt --title CodeWhale cmd /k "%~dp0codewhale-windows-x64.exe"', | ||
| " set NO_ANIMATIONS=1", | ||
| ") else (", | ||
| ' "%~dp0codewhale-windows-x64.exe"', | ||
| " set NO_ANIMATIONS=1", | ||
| ")", | ||
| "", | ||
| ].join("\r\n"); | ||
| const batPath = path.join(outputDir, "codewhale.bat"); | ||
| await fs.writeFile(batPath, batContent, "utf8"); | ||
| const batHash = await sha256(batPath); | ||
| manifestLines.push(`${batHash} codewhale.bat`); | ||
| console.log(`Generated ${batPath}`); | ||
| } |
There was a problem hiding this comment.
There are three issues with the current batch file generation:
- Environment Variable Timing:
set NO_ANIMATIONS=1is executed after the executable is launched. Since running a console application blocks the batch script, and launchingwtspawns a separate process immediately, the environment variable is never inherited by the runningcodewhaleprocess. It should be set at the top of the batch file. - Terminal Window Persistence: Using
cmd /kkeeps the command prompt window open aftercodewhaleexits. Changing this tocmd /censures the window closes automatically when the application terminates. - Cross-Platform Release Preparation: Guarding the generation with
if (isWindows)means that when preparing all assets on a non-Windows CI runner (e.g., Linux CI withprepareAllAssetsenabled),codewhale.batwill not be generated or included in the checksum manifest. Changing the guard toif (isWindows || prepareAllAssets)ensures the batch file is always packaged correctly.
if (isWindows || prepareAllAssets) {
const batContent = [
"@echo off",
"set NO_ANIMATIONS=1",
"where wt >nul 2>nul",
'if "%ERRORLEVEL%"=="0" (',
' wt --title CodeWhale cmd /c "%~dp0codewhale-windows-x64.exe"',
") else (",
' "%~dp0codewhale-windows-x64.exe"',
")",
"",
].join("\r\n");
const batPath = path.join(outputDir, "codewhale.bat");
await fs.writeFile(batPath, batContent, "utf8");
const batHash = await sha256(batPath);
manifestLines.push(batHash + " codewhale.bat");
console.log("Generated " + batPath);
}| win32: { | ||
| x64: ["codewhale-windows-x64.exe", "codewhale-tui-windows-x64.exe"], | ||
| x64: ["codewhale-windows-x64.exe", "codewhale-tui-windows-x64.exe", "codewhale.bat"], | ||
| }, |
There was a problem hiding this comment.
Adding "codewhale.bat" to ASSET_MATRIX has no functional effect because allAssetNames() and detectBinaryNames() only reference index 0 and 1 of the asset array. Furthermore, if allAssetNames() were updated to include all elements, it would cause prepare-local-release-assets.js (when prepareAllAssets is enabled) to incorrectly copy the compiled codewhale.exe binary onto codewhale.bat. Since codewhale.bat is already manually generated and added to the manifest in prepare-local-release-assets.js, it should be removed from ASSET_MATRIX to avoid confusion and potential bugs.
| win32: { | |
| x64: ["codewhale-windows-x64.exe", "codewhale-tui-windows-x64.exe"], | |
| x64: ["codewhale-windows-x64.exe", "codewhale-tui-windows-x64.exe", "codewhale.bat"], | |
| }, | |
| win32: { | |
| x64: ["codewhale-windows-x64.exe", "codewhale-tui-windows-x64.exe"], | |
| }, |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 244fd739b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ' wt --title CodeWhale cmd /k "%~dp0codewhale-windows-x64.exe"', | ||
| " set NO_ANIMATIONS=1", | ||
| ") else (", | ||
| ' "%~dp0codewhale-windows-x64.exe"', |
There was a problem hiding this comment.
When users run the generated Windows launcher with any arguments, such as codewhale.bat --version or a command prompt, these lines invoke the executable without %*, so the batch file silently drops every argument. This only affects users launching through the new .bat asset, but it makes that launcher behave differently from invoking codewhale-windows-x64.exe directly.
Useful? React with 👍 / 👎.
| ' wt --title CodeWhale cmd /k "%~dp0codewhale-windows-x64.exe"', | ||
| " set NO_ANIMATIONS=1", | ||
| ") else (", | ||
| ' "%~dp0codewhale-windows-x64.exe"', | ||
| " set NO_ANIMATIONS=1", |
There was a problem hiding this comment.
In both branches the executable is started before NO_ANIMATIONS is set, so the child process never sees the variable; in the wt case the new terminal is launched with the old environment, and in the fallback case cmd waits for the exe to exit before setting it. This means the generated launcher does not actually force the intended accessibility mode for Windows users.
Useful? React with 👍 / 👎.
| }, | ||
| win32: { | ||
| x64: ["codewhale-windows-x64.exe", "codewhale-tui-windows-x64.exe"], | ||
| x64: ["codewhale-windows-x64.exe", "codewhale-tui-windows-x64.exe", "codewhale.bat"], |
There was a problem hiding this comment.
Adding codewhale.bat as a third entry here does not make it part of allAssetNames() or allReleaseAssetNames(), because that helper only pushes pair[0] and pair[1]. I checked the release workflow: it publishes downloaded build artifacts and generates the manifest from those files, while the new batch file is only created by the local prepare script on Windows, so release:check/prepublishOnly still will not require or verify this asset.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const batContent = [ | ||
| "@echo off", | ||
| "where wt >nul 2>nul", | ||
| 'if "%ERRORLEVEL%"=="0" (', | ||
| ' wt --title CodeWhale cmd /k "%~dp0codewhale-windows-x64.exe"', | ||
| " set NO_ANIMATIONS=1", | ||
| ") else (", | ||
| ' "%~dp0codewhale-windows-x64.exe"', | ||
| " set NO_ANIMATIONS=1", | ||
| ")", | ||
| "", | ||
| ].join("\r\n"); |
There was a problem hiding this comment.
set NO_ANIMATIONS=1 is placed after the binary is launched in both branches, so the environment variable is never visible to the process. In the else branch the .exe runs synchronously to completion before set executes; in the wt branch, wt spawns a new terminal window immediately and returns, so set only mutates the current bat session's environment — it does not propagate into the already-started cmd /k child. Moving the set before the branch is the correct fix.
| const batContent = [ | |
| "@echo off", | |
| "where wt >nul 2>nul", | |
| 'if "%ERRORLEVEL%"=="0" (', | |
| ' wt --title CodeWhale cmd /k "%~dp0codewhale-windows-x64.exe"', | |
| " set NO_ANIMATIONS=1", | |
| ") else (", | |
| ' "%~dp0codewhale-windows-x64.exe"', | |
| " set NO_ANIMATIONS=1", | |
| ")", | |
| "", | |
| ].join("\r\n"); | |
| const batContent = [ | |
| "@echo off", | |
| "set NO_ANIMATIONS=1", | |
| "where wt >nul 2>nul", | |
| 'if "%ERRORLEVEL%"=="0" (', | |
| ' wt --title CodeWhale cmd /k "%~dp0codewhale-windows-x64.exe"', | |
| ") else (", | |
| ' "%~dp0codewhale-windows-x64.exe"', | |
| ")", | |
| "", | |
| ].join("\r\n"); |
1 participant
Closes #2300
Adds a "How to Choose a Provider" section and FAQ to PROVIDERS.md.
Key clarifications:
Greptile Summary
This PR adds a Windows
.batlauncher that prefers Windows Terminal and generates it as part of the release asset preparation, with a corresponding entry added toASSET_MATRIXinartifacts.js..batlauncher generation (prepare-local-release-assets.js): the newif (isWindows)block writescodewhale.bat, hashes it, and appends it to the checksum manifest — butset NO_ANIMATIONS=1is placed after the binary is launched in both branches, so the environment variable is never present when the process starts.ASSET_MATRIXupdate (artifacts.js):codewhale.batis added as a third element to thewin32/x64pair, butallAssetNames()(and thereforeallReleaseAssetNames()) only iteratespair[0]andpair[1], meaningcodewhale.batis excluded from release-asset verification even though it appears in the checksum manifest.Confidence Score: 2/5
Not safe to merge as-is: the
.batlauncher'sNO_ANIMATIONSvariable is set after the binary runs (making it a no-op), andcodewhale.batis invisible to the release-verification path due to the fixed-arity assumption inallAssetNames().Two concrete defects in the changed code: the env-var ordering means Windows Terminal users never get the intended
NO_ANIMATIONS=1behavior, and theallAssetNames()function's hard-codedpair[0]/pair[1]indexing silently dropscodewhale.batfrom release verification on every run.Both changed files need attention:
prepare-local-release-assets.jsfor thesetordering, andartifacts.jsfor the incomplete iteration inallAssetNames().Important Files Changed
set NO_ANIMATIONS=1is placed after the binary is invoked in both branches, so the env var is never seen by the process.codewhale.batas a third element to thewin32/x64asset pair, butallAssetNames()anddetectBinaryNames()only readpair[0]/pair[1], so the entry is invisible to release verification and install logic.Sequence Diagram
sequenceDiagram participant User participant codewhale.bat participant wt as Windows Terminal (wt) participant cmd as cmd /k participant exe as codewhale-windows-x64.exe User->>codewhale.bat: double-click / invoke codewhale.bat->>codewhale.bat: where wt alt wt found codewhale.bat->>wt: wt --title CodeWhale cmd /k exe wt->>cmd: spawn cmd /k cmd->>exe: run exe Note over codewhale.bat: set NO_ANIMATIONS=1 (too late) else wt not found codewhale.bat->>exe: run exe (blocks) exe-->>codewhale.bat: exits Note over codewhale.bat: set NO_ANIMATIONS=1 (too late) endComments Outside Diff (1)
npm/codewhale/scripts/artifacts.js, line 111-119 (link)allAssetNames()hard-codespair[0]andpair[1], socodewhale.bat(nowpair[2]of thewin32/x64entry) is silently omitted from the returned set. As a result,allReleaseAssetNames()also excludes it, meaningverify-release-assets.js(line 118) never checks whethercodewhale.batwas actually published to the GitHub release, even though the checksum manifest will contain its hash. Either extend the loop topair[2]when it exists, or keep the.batout ofASSET_MATRIXentirely and treat it as a purely generated artifact.Reviews (1): Last reviewed commit: "fix: Windows .bat launcher with correct ..." | Re-trigger Greptile