Conversation
📝 WalkthroughWalkthroughTwo new files are introduced to the repository: an updated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build-window.bat`:
- Line 1: This file (build-window.bat) currently uses LF-only line endings which
can break Windows batch parsing; convert build-window.bat to CRLF line endings
and add a repository .gitattributes entry to enforce CRLF for batch files (e.g.,
add a rule for *.bat text eol=crlf) so future commits keep correct line endings.
- Around line 25-27: The script currently stops the dagu service with "sc stop
dagu", runs "go build" without checking its result, and then always runs "sc
start dagu", which can leave the service stopped or restart an old binary; also
there is no admin privilege check and no wait for the service to fully stop. Fix
by adding an early privilege guard (use "net session" or equivalent to detect
elevation and exit with a clear message if not elevated), after "sc stop dagu"
poll "sc query dagu" until the service reports STOPPED (with a timeout/retry)
before attempting the build, run "go build" and capture %ERRORLEVEL%
immediately, only run "sc start dagu" if the build succeeded, and ensure the
script exits with the captured build error code so failures are propagated
(references: sc stop dagu, sc query dagu, go build, %ERRORLEVEL%, sc start dagu,
net session).
- Around line 1-27: The script build-window.bat has portability and robustness
issues: rename to build-windows.bat if matching convention, scope env changes
with setlocal/endlocal so setting CGO_ENABLED=0 doesn't leak, replace cd ui/cd
.. with pushd ui / popd to ensure the caller's cwd is restored on failure, avoid
the hardcoded output C:\usr\bin\dagu.exe by using a %OUT% or %VERSIONED_OUT% env
var with a sensible default like .\bin\dagu.exe, and stop hardcoding -X
main.version=3.0.0 by accepting a %VERSION% argument or deriving it via git
describe so the -ldflags value reflects the actual build version.
- Around line 14-20: The asset-cleanup step currently deletes the repo sentinel
`.gitkeep` and lacks error handling; update the deletion logic (the del and the
for /D %%d ... rmdir loop) to skip `.gitkeep` (e.g., delete using a file
wildcard that excludes .gitkeep or delete everything then recreate/restor e
`.gitkeep`) and add error checks on the destructive/copy steps by appending `||
exit /b` (or `if errorlevel 1 exit /b`) to the del command, the rmdir loop
operations, and the xcopy command so failures stop the script; look for the del,
for /D %%d ... rmdir, and xcopy lines to apply these changes.
🪄 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
Run ID: 868daef4-4a89-424b-8c9d-1055f0d73539
📒 Files selected for processing (2)
.gitignorebuild-window.bat
| @@ -0,0 +1,27 @@ | |||
| @echo off | |||
There was a problem hiding this comment.
Line endings: file is LF-only.
Flagged by static analysis: Windows batch files are sensitive to line endings and LF-only .bat files can exhibit label/goto parsing oddities due to the parser's 512-byte buffer behavior. Convert to CRLF and consider adding a .gitattributes entry:
*.bat text eol=crlf
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 27 LF sequences found
(E018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build-window.bat` at line 1, This file (build-window.bat) currently uses
LF-only line endings which can break Windows batch parsing; convert
build-window.bat to CRLF line endings and add a repository .gitattributes entry
to enforce CRLF for batch files (e.g., add a rule for *.bat text eol=crlf) so
future commits keep correct line endings.
| @echo off | ||
|
|
||
| REM Build UI assets | ||
| echo Building UI assets... | ||
|
|
||
| cd ui || exit /b | ||
|
|
||
| call pnpm install --frozen-lockfile || exit /b | ||
| call pnpm build || exit /b | ||
|
|
||
| cd .. | ||
|
|
||
| REM Delete all files | ||
| del /F /Q "internal\service\frontend\assets\*" 2>nul | ||
|
|
||
| REM Delete all subfolders | ||
| for /D %%d in ("internal\service\frontend\assets\*") do rmdir /S /Q "%%d" | ||
|
|
||
| echo "Copy built files" | ||
| xcopy /E /I /Y ui\dist\* internal\service\frontend\assets\ | ||
|
|
||
| REM set GOOS=windows | ||
| REM set GOARCH=amd64 | ||
| set CGO_ENABLED=0 | ||
| sc stop dagu | ||
| go build -ldflags="-s -w -X main.version=3.0.0" -o C:\usr\bin\dagu.exe ./cmd | ||
| sc start dagu |
There was a problem hiding this comment.
Several portability/robustness concerns with this new build script.
A few things worth addressing before merging:
- Filename typo:
build-window.batshould likely bebuild-windows.batto match the singular/plural convention used elsewhere (e.g.,scripts/build.ps1). - No
setlocal:set CGO_ENABLED=0leaks into the caller's environment. Wrap the script withsetlocal(and optionallyendlocal) to scope variable changes. - Hardcoded output path
C:\usr\bin\dagu.exeis unusual on Windows and non-portable across machines. Consider parameterizing via an env var with a sensible default (e.g.,.\bin\dagu.exe). - Hardcoded version
-X main.version=3.0.0drifts fromscripts/build.ps1which derives version from git ($BuildVersion-$Date). This will silently report the wrong version for any release not literally 3.0.0. Prefergit describeor accept a%VERSION%arg. cd uiwithoutpopd: ifpnpm install/pnpm buildfails,exit /bleaves the caller inui\. Usepushd ui/popdinstead ofcd ui/cd ...
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 27 LF sequences found
(E018)
[error] 17-17: Dangerous command without confirmation. Explanation: Destructive commands should require user confirmation to prevent accidental execution. Recommendation: Add confirmation prompts before destructive operations. Context: Destructive command should have user confirmation
(SEC003)
[error] 25-25: Missing privilege check. Explanation: Operations requiring admin rights should check for proper privileges. Recommendation: Use NET SESSION >nul 2>&1 to check for administrator privileges. Context: Command 'sc' may require administrator privileges
(SEC005)
[error] 11-11: Unvalidated path traversal. Explanation: Path operations may allow directory traversal attacks with .. sequences. Recommendation: Validate paths and remove .. sequences before file operations. Context: Path contains .. which may allow directory traversal
(SEC011)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build-window.bat` around lines 1 - 27, The script build-window.bat has
portability and robustness issues: rename to build-windows.bat if matching
convention, scope env changes with setlocal/endlocal so setting CGO_ENABLED=0
doesn't leak, replace cd ui/cd .. with pushd ui / popd to ensure the caller's
cwd is restored on failure, avoid the hardcoded output C:\usr\bin\dagu.exe by
using a %OUT% or %VERSIONED_OUT% env var with a sensible default like
.\bin\dagu.exe, and stop hardcoding -X main.version=3.0.0 by accepting a
%VERSION% argument or deriving it via git describe so the -ldflags value
reflects the actual build version.
| del /F /Q "internal\service\frontend\assets\*" 2>nul | ||
|
|
||
| REM Delete all subfolders | ||
| for /D %%d in ("internal\service\frontend\assets\*") do rmdir /S /Q "%%d" | ||
|
|
||
| echo "Copy built files" | ||
| xcopy /E /I /Y ui\dist\* internal\service\frontend\assets\ |
There was a problem hiding this comment.
Asset wipe may delete .gitkeep and lacks error handling.
del /F /Q "internal\service\frontend\assets\*"on line 14 will remove the.gitkeepfile that the repo explicitly preserves (see.gitignorerule!internal/service/frontend/assets/.gitkeep). After this script runs, the sentinel is gone from the working tree. Either exclude it (... && del ... /A:-Dplus a filter) or restore it afterxcopy.- Lines 14, 17, and 20 do not check
errorlevel. Ifxcopyfails, the script silently proceeds to build and (re)start the service with empty/partial embedded assets. Append|| exit /b(or equivalentif errorlevel 1 exit /b) to each destructive/copy step.
🧰 Tools
🪛 Blinter (1.0.112)
[error] 17-17: Dangerous command without confirmation. Explanation: Destructive commands should require user confirmation to prevent accidental execution. Recommendation: Add confirmation prompts before destructive operations. Context: Destructive command should have user confirmation
(SEC003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build-window.bat` around lines 14 - 20, The asset-cleanup step currently
deletes the repo sentinel `.gitkeep` and lacks error handling; update the
deletion logic (the del and the for /D %%d ... rmdir loop) to skip `.gitkeep`
(e.g., delete using a file wildcard that excludes .gitkeep or delete everything
then recreate/restor e `.gitkeep`) and add error checks on the destructive/copy
steps by appending `|| exit /b` (or `if errorlevel 1 exit /b`) to the del
command, the rmdir loop operations, and the xcopy command so failures stop the
script; look for the del, for /D %%d ... rmdir, and xcopy lines to apply these
changes.
| sc stop dagu | ||
| go build -ldflags="-s -w -X main.version=3.0.0" -o C:\usr\bin\dagu.exe ./cmd | ||
| sc start dagu |
There was a problem hiding this comment.
Service left stopped on build failure; no admin/privilege guard.
go buildon line 26 has no error check. If compilation fails, the script falls through tosc start dagu, which will start the old binary — but worse, if any earlier step failed aftersc stop dagu, the service stays down silently. Gate line 27 on build success and ensure the service is always attempted to be restarted on exit:sc stop dagu go build -ldflags="-s -w -X main.version=3.0.0" -o C:\usr\bin\dagu.exe ./cmd set BUILD_ERR=%ERRORLEVEL% sc start dagu exit /b %BUILD_ERR%
sc stop/sc startrequire administrator privileges. Consider adding an early privilege check (e.g.,net session >nul 2>&1 || (echo Run as Administrator & exit /b 1)) so non-elevated runs fail fast with a clear message rather than a crypticscerror.- Additionally, writing to
C:\usr\bin\dagu.exewhile the service has just stopped assumes the file is unlocked. Ifsc stopreturns before the process fully exits,go buildmay fail with a file-in-use error. Consider pollingsc query daguforSTOPPEDbefore proceeding.
🧰 Tools
🪛 Blinter (1.0.112)
[error] 25-25: Missing privilege check. Explanation: Operations requiring admin rights should check for proper privileges. Recommendation: Use NET SESSION >nul 2>&1 to check for administrator privileges. Context: Command 'sc' may require administrator privileges
(SEC005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build-window.bat` around lines 25 - 27, The script currently stops the dagu
service with "sc stop dagu", runs "go build" without checking its result, and
then always runs "sc start dagu", which can leave the service stopped or restart
an old binary; also there is no admin privilege check and no wait for the
service to fully stop. Fix by adding an early privilege guard (use "net session"
or equivalent to detect elevation and exit with a clear message if not
elevated), after "sc stop dagu" poll "sc query dagu" until the service reports
STOPPED (with a timeout/retry) before attempting the build, run "go build" and
capture %ERRORLEVEL% immediately, only run "sc start dagu" if the build
succeeded, and ensure the script exits with the captured build error code so
failures are propagated (references: sc stop dagu, sc query dagu, go build,
%ERRORLEVEL%, sc start dagu, net session).
|
Thank you very much @samaune for adding the nice script! Would you mind if you address the review comments from code rabbit? |
2 participants
Summary by CodeRabbit