fix: windows command execution#40
Merged
Merged
Conversation
- Remove const scriptHeader '#!/bin/bash' from script.go - GenerateScript() returns body without shebang prefix, empty input returns empty string - ParseScriptBody() generically strips any #! prefix line for backward compat - Both old (#!/bin/bash in DB) and new (no shebang) scripts handled transparently
…sage - Add platform-appropriate shebang in ExecuteScript: #!/bin/sh on Unix, none on Windows - Strip existing shebangs from stored content before adding platform shebang (backward compat) - Fix Unix execution path to use -lc flag (was previously dropped, cmd ran without login shell) - Add stripShebang helper for generic #! line removal - Both platforms now uniformly use e.flag in exec.CommandContext
- Remove hardcoded #!/bin/bash from stored scripts - Platform-aware shebang injection at execution time (#!/bin/sh on Unix, none on Windows) - Fix Unix -lc flag usage for login shell - Generic #! stripping for backward compat with old DB records - No changes needed to execution_service.go (callers transparently compatible)
…form-appropriate shell name - Convert BuildFinalCommand from package-level function to method on *Executor - Use e.shell basename instead of hardcoded "bash" - Windows displays "cmd <script>", Unix displays "sh"/"zsh"/"bash"/" <script>" per $SHELL - Fallback to "sh" if shell name is empty
…y strings plan - Convert BuildFinalCommand to *Executor method with e.shell basename - No more hardcoded 'bash' in display output - BuildFinalCommand confirmed dead code (zero callers); BuildDisplayCommand handles execution history display - STATE.md updated: plan 2/3 complete, decisions recorded
…ctions covering GenerateScript, ParseScriptBody, ExtractTemplateVars, ReplaceTemplateVars, MergeDetectedVars - TestGenerateScript: verifies no shebang prefix, handles empty/whitespace/multi-line - TestParseScriptBody: verifies backward compat with #!/bin/bash and #!/usr/bin/env bash, no-shebang format, empty, shebang-only - TestExtractTemplateVars: verifies variable extraction and deduplication - TestReplaceTemplateVars: verifies template substitution with known and unknown vars - TestMergeDetectedVars: verifies merge order (detected first, then manual-only) - Fix ParseScriptBody edge case: shebang-only input now returns empty string
…project checks - Created script_test.go with 5 test functions covering script generation, parsing, template vars - Fixed ParseScriptBody edge case for shebang-only inputs - All 8 Go tests pass, make check passes (go build + pnpm tsc) - Wails bindings verified up to date
-c flag treats the file path as a command string, requiring +x permission. -l alone tells the shell to read the file as a login shell script. Only affects ExecuteScript; terminal launchers use -lc correctly with command strings.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughExecutor and script handling were changed to generalize shebang stripping and unify script execution. ChangesScript / Executor Shebang and Execution
Sequence DiagramsequenceDiagram
participant Client
participant Executor
participant FS as FileSystem
participant Shell
Client->>Executor: request to run scriptContent + vars
Executor->>Executor: stripShebang(scriptContent)
alt non-Windows
Executor->>FS: write temp file with "#!/bin/sh\n" + body
else Windows
Executor->>FS: write temp file with body
end
Executor->>Shell: start process `e.shell` with `e.flag` and tempPath
Shell->>FS: read/execute tempPath
Shell-->>Executor: exit code / output
Executor-->>Client: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
script_test.go (1)
1-129: ⚡ Quick winKeep new Go tests in
db_test.go, or update the guideline in this PR too.These cases are valuable, but adding
script_test.godiverges from the current repo test-file contract. If the project is intentionally moving away from that layout, the guideline should change in the same PR.As per coding guidelines, "
*_test.go: Go tests are indb_test.goonly; run withgo test ./...; three core migration tests exist".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script_test.go` around lines 1 - 129, The new tests in script_test.go (TestGenerateScript, TestParseScriptBody, TestExtractTemplateVars, TestReplaceTemplateVars, TestMergeDetectedVars) violate the repo guideline that all Go tests belong in db_test.go; either move these test functions into db_test.go (preserving their names and test logic and updating imports if needed) or update the repository guideline in this PR to accept additional *_test.go files; ensure you modify the same PR to reflect the chosen approach so the repo contract remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@executor.go`:
- Around line 113-116: The current Unix path injects a "#!/bin/sh" and then
spawns the shell with the temp-file as an argument, which breaks the executor
contract; instead, remove the shebang injection (do not prepend "#!/bin/sh" to
scriptContent) and on non-Windows use the user's shell: pick shell :=
os.Getenv("SHELL"); if empty set shell = "/bin/sh", then exec shell with args
"-lc" and stream scriptContent to the shell's stdin (so the shell interprets the
script body), while on Windows use the cmd "/C" execution path as before; adjust
the code that builds and runs the command (references: scriptContent and the
runtime.GOOS check) to implement this behavior.
- Around line 219-229: stripShebang currently returns the raw shebang line when
the content contains only a shebang without a trailing newline, causing a
shebang-only stored script to be executed; update stripShebang (the function
named stripShebang) to mirror ParseScriptBody behavior by returning an empty
string when a shebang is present but no newline follows (i.e., if
strings.HasPrefix(s, "#!") and strings.Index(s, "\n") == -1 then return ""),
otherwise continue returning the text after the first newline as before.
In `@script.go`:
- Around line 11-18: GenerateScript currently strips all whitespace and returns
only the body, breaking the storage contract that persisted scripts must include
the "#!/bin/bash" shebang; change GenerateScript so that for non-empty bodies it
trims whitespace but prepends the shebang line ("#!/bin/bash\n") before
returning, while leaving ParseScriptBody responsible for removing the shebang
for the editor/runtime split; update GenerateScript to return "" for empty
bodies and "#!/bin/bash\n" + body + "\n" (or equivalent) for non-empty scripts
to preserve the persisted format.
---
Nitpick comments:
In `@script_test.go`:
- Around line 1-129: The new tests in script_test.go (TestGenerateScript,
TestParseScriptBody, TestExtractTemplateVars, TestReplaceTemplateVars,
TestMergeDetectedVars) violate the repo guideline that all Go tests belong in
db_test.go; either move these test functions into db_test.go (preserving their
names and test logic and updating imports if needed) or update the repository
guideline in this PR to accept additional *_test.go files; ensure you modify the
same PR to reflect the chosen approach so the repo contract remains consistent.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8bba1f81-279a-4c47-8874-38d1553e893b
⛔ Files ignored due to path filters (2)
.planning/ROADMAP.mdis excluded by!.planning/*.planning/STATE.mdis excluded by!.planning/*
📒 Files selected for processing (3)
executor.goscript.goscript_test.go
Comment on lines
+113
to
+116
| // Add platform-appropriate shebang at execution time | ||
| if runtime.GOOS != "windows" { | ||
| scriptContent = "#!/bin/sh\n" + scriptContent | ||
| } |
Contributor
There was a problem hiding this comment.
Restore the Unix $SHELL -lc execution path.
Line 131 hardcodes -l and passes the temp file as a script argument. That no longer honors the executor contract, and the injected #!/bin/sh is ignored because the user's shell parses the file itself. On non-POSIX login shells, valid script bodies will now fail.
As per coding guidelines, "Platform-aware shell execution must use $SHELL -lc on Unix (falls back to /bin/sh), cmd /C on Windows".
Also applies to: 127-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor.go` around lines 113 - 116, The current Unix path injects a
"#!/bin/sh" and then spawns the shell with the temp-file as an argument, which
breaks the executor contract; instead, remove the shebang injection (do not
prepend "#!/bin/sh" to scriptContent) and on non-Windows use the user's shell:
pick shell := os.Getenv("SHELL"); if empty set shell = "/bin/sh", then exec
shell with args "-lc" and stream scriptContent to the shell's stdin (so the
shell interprets the script body), while on Windows use the cmd "/C" execution
path as before; adjust the code that builds and runs the command (references:
scriptContent and the runtime.GOOS check) to implement this behavior.
Comment on lines
+11
to
+18
| // GenerateScript returns the script body trimmed of surrounding whitespace with a trailing newline. | ||
| // No shebang is prepended — the executor adds a platform-appropriate shebang at execution time. | ||
| func GenerateScript(body string) string { | ||
| body = strings.TrimSpace(body) | ||
| return scriptHeader + "\n\n" + body + "\n" | ||
| if body == "" { | ||
| return "" | ||
| } | ||
| return body + "\n" |
Contributor
There was a problem hiding this comment.
Preserve the persisted #!/bin/bash header.
GenerateScript now stores a bare body, which changes the DB/storage contract and makes the editor/runtime split inconsistent. Keep stripping the header in ParseScriptBody, but still prepend #!/bin/bash when persisting non-empty scripts.
Suggested fix
func GenerateScript(body string) string {
body = strings.TrimSpace(body)
if body == "" {
return ""
}
- return body + "\n"
+ return "#!/bin/bash\n" + body + "\n"
}As per coding guidelines, "Scripts must be stored with #!/bin/bash shebang; the editor shows/edits the body without the shebang".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // GenerateScript returns the script body trimmed of surrounding whitespace with a trailing newline. | |
| // No shebang is prepended — the executor adds a platform-appropriate shebang at execution time. | |
| func GenerateScript(body string) string { | |
| body = strings.TrimSpace(body) | |
| return scriptHeader + "\n\n" + body + "\n" | |
| if body == "" { | |
| return "" | |
| } | |
| return body + "\n" | |
| // GenerateScript returns the script body trimmed of surrounding whitespace with a trailing newline. | |
| // No shebang is prepended — the executor adds a platform-appropriate shebang at execution time. | |
| func GenerateScript(body string) string { | |
| body = strings.TrimSpace(body) | |
| if body == "" { | |
| return "" | |
| } | |
| return "#!/bin/bash\n" + body + "\n" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script.go` around lines 11 - 18, GenerateScript currently strips all
whitespace and returns only the body, breaking the storage contract that
persisted scripts must include the "#!/bin/bash" shebang; change GenerateScript
so that for non-empty bodies it trims whitespace but prepends the shebang line
("#!/bin/bash\n") before returning, while leaving ParseScriptBody responsible
for removing the shebang for the editor/runtime split; update GenerateScript to
return "" for empty bodies and "#!/bin/bash\n" + body + "\n" (or equivalent) for
non-empty scripts to preserve the persisted format.
…ation - ExecuteScript: pipe body to shell -lc stdin on Unix (no temp file needed, no shebang injection, no permission issues). Windows unchanged. - stripShebang: return empty for shebang-only content (mirrors ParseScriptBody behavior), preventing execution of empty scripts. - Move 5 test functions from script_test.go into db_test.go per repo convention.
-c requires a command string argument; piping via stdin needs -s (read from stdin). Combined with -l for login shell behavior.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
executor.go (1)
30-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't bypass the configured Unix shell flag.
NewExecutorestablishes-lcas the Unix execution contract, butExecuteScripthardcodes-lshere. That makes this path diverge from the rest of the executor configuration and from the repository's Unix-shell requirement.#!/bin/bash set -euo pipefail # Verify the configured Unix flag and the flag actually used by ExecuteScript. rg -n --type=go 'flag = "-lc"|exec\.CommandContext\(ctx, e\.shell, "-ls"\)|exec\.CommandContext\(ctx, e\.shell, e\.flag' executor.goAs per coding guidelines, "Platform-aware shell execution must use $SHELL -lc on Unix (falls back to /bin/sh), cmd /C on Windows".
Also applies to: 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor.go` around lines 30 - 42, ExecuteScript is hardcoding the Unix shell flag ("-ls") instead of using the configured e.flag set by NewExecutor, which breaks the repo's requirement to use $SHELL -lc (or /bin/sh fallback) and cmd /C on Windows; update all invocations of exec.CommandContext(ctx, e.shell, "-ls", ...) (and any other places where "-ls" is used) to pass e.flag (i.e., exec.CommandContext(ctx, e.shell, e.flag, ...)) so the executor consistently uses the configured shell flag, and verify any other exec.CommandContext calls in the file also use e.flag rather than hardcoded flags.
🧹 Nitpick comments (1)
db_test.go (1)
176-180: ⚡ Quick winAdd the no-trailing-newline shebang case.
This only covers
#!/bin/bash\n. The regression fixed instripShebangwas#!/bin/bashwith no newline, so this suite still won't catch that edge case.Suggested test addition
// Only shebang, no body result = ParseScriptBody("#!/bin/bash\n") if result != "" { t.Errorf("ParseScriptBody shebang only: got %q, want empty", result) } + + result = ParseScriptBody("#!/bin/bash") + if result != "" { + t.Errorf("ParseScriptBody shebang only without newline: got %q, want empty", result) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db_test.go` around lines 176 - 180, Add a test case in db_test.go to cover a shebang with no trailing newline by calling ParseScriptBody with "#!/bin/bash" (no "\n") and asserting the result is empty; update the existing test block that checks shebang-only (the one using ParseScriptBody) to include this no-newline variant so ParseScriptBody/stripShebang regression is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@executor.go`:
- Around line 214-225: OpenInTerminal contains its own inline shebang-stripping
logic that can still launch a shebang-only string as a command; replace that
logic by calling the existing stripShebang(content string) function (the same
one used by ExecuteScript) so shebang-only content becomes empty before
launching. Locate OpenInTerminal and any duplicate logic around lines ~229-236
and remove the inline trimming/HasPrefix/Index handling, call stripShebang on
the script content, and ensure the resulting string is checked for emptiness
exactly as ExecuteScript does before attempting to execute/open the terminal.
---
Duplicate comments:
In `@executor.go`:
- Around line 30-42: ExecuteScript is hardcoding the Unix shell flag ("-ls")
instead of using the configured e.flag set by NewExecutor, which breaks the
repo's requirement to use $SHELL -lc (or /bin/sh fallback) and cmd /C on
Windows; update all invocations of exec.CommandContext(ctx, e.shell, "-ls", ...)
(and any other places where "-ls" is used) to pass e.flag (i.e.,
exec.CommandContext(ctx, e.shell, e.flag, ...)) so the executor consistently
uses the configured shell flag, and verify any other exec.CommandContext calls
in the file also use e.flag rather than hardcoded flags.
---
Nitpick comments:
In `@db_test.go`:
- Around line 176-180: Add a test case in db_test.go to cover a shebang with no
trailing newline by calling ParseScriptBody with "#!/bin/bash" (no "\n") and
asserting the result is empty; update the existing test block that checks
shebang-only (the one using ParseScriptBody) to include this no-newline variant
so ParseScriptBody/stripShebang regression is exercised.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ba77355-a05a-451e-91e9-89d9829c207a
📒 Files selected for processing (2)
db_test.goexecutor.go
… add test - OpenInTerminal: replace inline shebang stripping with stripShebang() call, fixing shebang-only edge case where raw #! would be passed to terminal. - ExecuteScript: use e.flag consistently on all platforms. Unix path now writes temp file with #!/bin/sh shebang + chmod 0755 so -lc can exec it. - db_test.go: add ParseScriptBody test for shebang with no trailing newline.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests