fix: restore simple Windows installer command - #2541
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Windows installer now runs in an isolated PowerShell script block. Windows CI performs real PowerShell and CMD installation checks. Coordinator heartbeats preserve cancellation and deadline errors. Proc-store filesystem access uses shared utilities. ChangesWindows installer validation
Coordinator heartbeat timeout handling
Proc-store filesystem access
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI as Windows CI
participant Installer as installer.ps1
participant System as Installed system
CI->>Installer: Invoke downloaded installer expression
Installer->>System: Install executable
CI->>System: Check executable version
CI->>Installer: Run cleanup and invalid-flag checks
sequenceDiagram
participant Heartbeat as Coordinator heartbeat
participant Monitor as Service monitor
participant RPC as Coordinator heartbeat RPC
Heartbeat->>Monitor: Discover service members
Monitor-->>Heartbeat: Return members or context error
Heartbeat->>RPC: Send heartbeat with request context
RPC-->>Heartbeat: Return heartbeat result
Heartbeat-->>Heartbeat: Normalize cancellation or deadline error
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 1
🧹 Nitpick comments (1)
scripts/installer.ps1 (1)
6-7: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify that all invocation paths preserve the same isolation contract.
The wrapper isolates local parameters, but the installer also uses
$Script:state. The implementation and test must prove that string-based invocations cannot modify caller script state.
scripts/installer.ps1#L6-L7: verify the non-file staging decision before$Script:state is initialized.scripts/installer.ps1#L1257-L1257: verify@argsforwarding and preserve the original error and exit behavior..github/workflows/ci.yaml#L366-L374: assert$script:Serviceand$script:DaguHome, in addition to local$Service.🤖 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 `@scripts/installer.ps1` around lines 6 - 7, Preserve installer isolation for every invocation path: in scripts/installer.ps1 lines 6-7, determine non-file staging before initializing any $Script: state; at line 1257, forward arguments through `@args` while preserving the existing error and exit behavior; in .github/workflows/ci.yaml lines 366-374, extend the test assertions to verify $script:Service and $script:DaguHome alongside local $Service.
🤖 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 `@scripts/installer.ps1`:
- Around line 6-7: Save scripts/installer.ps1 as UTF-8 with BOM, preserving its
existing content and encoding all non-ASCII characters correctly. Use make
addlicense or the repository’s established license workflow to apply the
required BOM.
---
Nitpick comments:
In `@scripts/installer.ps1`:
- Around line 6-7: Preserve installer isolation for every invocation path: in
scripts/installer.ps1 lines 6-7, determine non-file staging before initializing
any $Script: state; at line 1257, forward arguments through `@args` while
preserving the existing error and exit behavior; in .github/workflows/ci.yaml
lines 366-374, extend the test assertions to verify $script:Service and
$script:DaguHome alongside local $Service.
🪄 Autofix
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: d7d82d9d-91e1-4682-bb7c-2e7f4928681d
📒 Files selected for processing (3)
.github/workflows/ci.yamlREADME.mdscripts/installer.ps1
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 @.github/workflows/ci.yaml:
- Around line 365-366: Update the installerUrl construction in the
test-windows-installer workflow to use
github.event.pull_request.head.repo.full_name for pull request runs, while
retaining GITHUB_REPOSITORY for non-PR runs. Keep installerRef aligned with the
corresponding head commit so the raw URL resolves to the correct repository and
revision.
🪄 Autofix
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: ae1dc96e-77ee-4e0f-8433-aab185ac36e6
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/installer.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/installer.ps1
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/coordinator/client_test.go (1)
1257-1258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct-call and retry-exhaustion coverage.
The RPC mock now returns an immediate gRPC error, and both timeout tests block service-member discovery. The changed tests therefore do not exercise
heartbeatContextErrorafterattemptCallat Line 866 or after retry exhaustion at Lines 888-901. Add deterministic tests that block the mock heartbeat onctx.Done(), with one coordinator and with multiple coordinators. Add a cancellation case if no existing test coverscontext.Canceled.Also applies to: 1343-1348, 1374-1379
🤖 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 `@internal/service/coordinator/client_test.go` around lines 1257 - 1258, Extend the heartbeat timeout tests around heartbeatContextError and attemptCall to block the mock heartbeat until ctx.Done(), rather than returning an immediate gRPC error. Add deterministic coverage with one coordinator for the direct-call path and multiple coordinators for retry exhaustion, ensuring discovery is blocked as required; also add a context.Canceled case if no existing test covers it.
🤖 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.
Nitpick comments:
In `@internal/service/coordinator/client_test.go`:
- Around line 1257-1258: Extend the heartbeat timeout tests around
heartbeatContextError and attemptCall to block the mock heartbeat until
ctx.Done(), rather than returning an immediate gRPC error. Add deterministic
coverage with one coordinator for the direct-call path and multiple coordinators
for retry exhaustion, ensuring discovery is blocked as required; also add a
context.Canceled case if no existing test covers it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57cc6333-8e2f-4b61-bdc0-0f05e2e67722
📒 Files selected for processing (2)
internal/service/coordinator/client.gointernal/service/coordinator/client_test.go
|
Addressed the latest CodeRabbit nitpick in 5919d1f: added deterministic single-coordinator timeout, multi-coordinator exhaustion, and caller-cancellation coverage. @coderabbitai review |
|
✅ Action performedReview finished.
|
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 `@internal/service/coordinator/client_test.go`:
- Around line 1403-1429: Update
TestClientHeartbeatReturnsDeadlineAfterFailoverExhaustion to count heartbeat
invocations across both mock coordinators, using shared tracking in mockCoord or
the heartbeat handlers. After client.Heartbeat returns, assert the total count
is two while retaining the context.DeadlineExceeded assertion; do not assert
coordinator call order because Heartbeat shuffles members.
🪄 Autofix
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: 4c7cbf4e-33a7-4a2c-9d7f-2bab0a574b86
📒 Files selected for processing (1)
internal/service/coordinator/client_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@internal/persis/file/proc/store_windows_test.go`:
- Around line 43-53: Ensure the handle created in this test is closed exactly
once: replace the independent t.Cleanup and release-goroutine closes with
synchronized ownership using sync.Once, or transfer cleanup ownership to the
goroutine after successful release. Update the CloseHandle calls around the
existing released channel so both paths cannot close the same handle
independently.
🪄 Autofix
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: cc09f9fd-5a6c-4846-819b-60e9a1797984
📒 Files selected for processing (2)
internal/persis/file/proc/store.gointernal/persis/file/proc/store_windows_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
irm ... | iexcommandRoot cause
Invoke-Expressionevaluates downloaded code in the caller's scope. The installer's top-level parameter metadata could therefore collide with an existing caller variable such as$Service. Renaming that parameter would only move the collision risk.The installer now creates its own child scope and forwards direct script arguments with
@args. Its source remains ASCII-safe so Windows PowerShell can execute both a downloaded file and the directirm ... | iexpipeline.Testing
GOCACHE=/private/tmp/dagu6-go-cache GOLANGCI_LINT_CACHE=/private/tmp/dagu6-golangci-cache make checkGOCACHE=/private/tmp/dagu6-go-cache go test -race ./internal/service/coordinator -count=10irm ... | iexpipeline, installs Dagu, runsdagu.exe version, uninstalls it, and verifies cleanupinstaller.cmdCloses #2530
Summary by CodeRabbit
Bug Fixes
Installation