[closed by author] accidental branch push — not a real change - #5362
[closed by author] accidental branch push — not a real change#5362Mustaqeem66 wants to merge 7 commits into
Conversation
pnpm runs package.json scripts through `cmd.exe /d /s /c`, which strips
the first and last quote characters from the command string. The
`dev:app:win` body quoted an absolute bash.exe path containing a space,
so cmd parsed `C:/Program` as the program name and the script failed on
every default Git for Windows install:
'C:/Program' is not recognized as an internal or external command
Quoting works normally inside a .cmd file, so route the entry point
through a new scripts/run-dev-win.cmd. package.json now references a
relative, space-free path that needs no quoting, and the wrapper quotes
the bash.exe path itself. This mirrors the .bat shim run-dev-win.sh
already generates internally for cargo-tauri's beforeDevCommand.
The wrapper probes the standard Git for Windows locations plus the
parent of git.exe on PATH, and honours OPENHUMAN_BASH_EXE for portable
installs. It deliberately avoids a bare `bash` lookup, which resolves to
the WSL launcher in System32 when WSL is enabled.
Closes tinyhumansai#5270
An unescaped `)` inside `%ProgramFiles(x86)%` in the diagnostic echo would have closed the enclosing `if (...)` block early, so the guidance never printed correctly. Jump to a label instead, where parentheses are literal. Refs tinyhumansai#5270
Collapse call expressions that fit within the repo's printWidth of 100 and replace the multi-line concatenation with straight-line statements, so `pnpm --filter openhuman-app format:check` is stable. Refs tinyhumansai#5270
The repository normalises everything to LF via `* text=auto eol=lf`, and `*.ps1` is the only Windows exception. cmd.exe's parser is line-ending sensitive -- LF-only batch files can mis-parse `goto`/`call :label` because of the 512-byte read-boundary bug -- so batch scripts need the same treatment PowerShell already gets. Refs tinyhumansai#5270
Deriving the Git root from `where git.exe` does not work under scoop: PATH only ever contains the shim directory, so `<shim dir>\..\bin\bash.exe` resolves to `scoop\bin\bash.exe`, which does not exist. The real binary lives under `scoop\apps\git\current\bin`, so probe that directly for the user-scope, global and relocated (`%SCOOP%`) install roots. Refs tinyhumansai#5270
📝 WalkthroughWalkthroughThe Windows development command now uses a new ChangesWindows development launcher
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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. Comment Warning |
Mustaqeem66
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@app/test/dev-app-win-launcher.test.ts`:
- Around line 1-10: Move the Vitest test from app/test into the related app/src
feature directory, preserving its *.test.ts name and behavior. After relocating
it, update the HERE-derived APP_DIR and REPO_ROOT path calculations in the test
so they still resolve app/package.json and the repository scripts directory
correctly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e673fd2c-5f4d-4422-8720-45104c191624
📒 Files selected for processing (5)
.gitattributesapp/package.jsonapp/test/dev-app-win-launcher.test.tscrlf-probe.txtscripts/run-dev-win.cmd
| import { existsSync, readFileSync } from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| const HERE = path.dirname(fileURLToPath(import.meta.url)); | ||
| const APP_DIR = path.resolve(HERE, '..'); | ||
| const REPO_ROOT = path.resolve(APP_DIR, '..'); | ||
| const PACKAGE_JSON_PATH = path.join(APP_DIR, 'package.json'); | ||
| const LAUNCHER_PATH = path.join(REPO_ROOT, 'scripts', 'run-dev-win.cmd'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move this Vitest test under app/src/**.
This new test is under app/test/. Move it to the related app/src/** feature location. Update APP_DIR and REPO_ROOT after the move.
As per coding guidelines, co-locate Vitest unit tests as *.test.ts(x) under app/src/**.
🤖 Prompt for 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.
In `@app/test/dev-app-win-launcher.test.ts` around lines 1 - 10, Move the Vitest
test from app/test into the related app/src feature directory, preserving its
*.test.ts name and behavior. After relocating it, update the HERE-derived
APP_DIR and REPO_ROOT path calculations in the test so they still resolve
app/package.json and the repository scripts directory correctly.
Source: Coding guidelines
|
| Filename | Overview |
|---|---|
| .gitattributes | Adds eol=crlf rules for *.cmd and *.bat; correct and necessary for the new launcher to function on Windows. |
| app/package.json | Replaces the broken bash-direct invocation (space-in-path cmd /S split) with a relative, no-spaces path to the new .cmd wrapper; fix is correct. |
| app/test/dev-app-win-launcher.test.ts | Well-structured regression tests covering the historical failure mode, single-token requirement, file existence, and WSL/bare-bash avoidance. |
| crlf-probe.txt | Temporary debug probe file (two lines: "line1", "line2") accidentally committed; should be deleted before merging. |
| scripts/run-dev-win.cmd | Carefully written CMD launcher that probes multiple Git for Windows install locations, quotes all paths correctly, and avoids the WSL bash.exe trap; minor edge case if %SCOOP% is undefined but covered by subsequent path checks. |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant pnpm
participant cmd as cmd.exe /d /s /c
participant wrapper as scripts/run-dev-win.cmd
participant bash as bash.exe (Git for Windows)
participant sh as scripts/run-dev-win.sh
Dev->>pnpm: pnpm dev:app:win
pnpm->>cmd: invoke script body
Note over cmd: resolves ..\\scripts\\run-dev-win.cmd<br/>(no spaces, no quoting needed)
cmd->>wrapper: execute run-dev-win.cmd
wrapper->>wrapper: probe OPENHUMAN_BASH_EXE
wrapper->>wrapper: probe %ProgramFiles%\\Git\\bin\\bash.exe
wrapper->>wrapper: probe %LOCALAPPDATA%\\Programs\\Git\\bin\\bash.exe
wrapper->>wrapper: probe Scoop paths
wrapper->>wrapper: derive from git.exe on PATH
alt bash.exe found
wrapper->>bash: ""%BASH_EXE%" "%SCRIPT_DIR%run-dev-win.sh" %*"
bash->>sh: execute run-dev-win.sh
else bash.exe not found
wrapper-->>Dev: error: install Git for Windows or set OPENHUMAN_BASH_EXE
end
Reviews (1): Last reviewed commit: "chore: crlf write probe" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| line1 | ||
| line2 |
There was a problem hiding this comment.
crlf-probe.txt contains only line1 and line2 — it is a throwaway file used during development to verify that the new .gitattributes rule was actually applying CRLF endings to .cmd/.bat files. The PR branch name (Tmp/crlf write probe) confirms this was never meant to land in main. Committing it adds noise to the tree and undermines the tidy scope of the fix.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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: 7dad1f3c73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| rem pnpm runs package.json scripts through `cmd.exe /d /s /c <string>`. The /S | ||
| rem flag strips the first and last quote characters from <string> before |
There was a problem hiding this comment.
In this new .cmd launcher, the rem comments contain literal <...> placeholders here and again in the Scoop/root comments below. Windows cmd still parses redirection metacharacters in batch comments; Microsoft’s rem docs state that redirection characters like < and > cannot be used in batch-file comments. On Windows, pnpm dev:app:win will parse these as redirections before the bash lookup runs, causing confusing startup errors and possible redirect-file side effects; escape them as ^<...^> or avoid angle brackets in the comments.
Useful? React with 👍 / 👎.
1 participant
Closing immediately — this was an accidental auto-opened PR from a scratch branch, not a proposed change. Sorry for the noise.
My real contribution is #5342. Nothing here should be reviewed.