add windows install script#6
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Windows (PowerShell) installer to bootstrap and run the Fivetran Proxy Agent via Docker, aligning Windows installation with the existing Unix install.sh flow.
Changes:
- Introduces
install.ps1to validate Docker prerequisites and host resources on Windows. - Bootstraps agent configuration (copy from file or fetch via
/proxy-agent/configure) and pins the latest available agent version. - Downloads the Windows manager script, starts the agent container, and reports health/status.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
This review was generated by Claude (claude-sonnet-4-6) using a multi-agent workflow that ran independent finders across several correctness/security angles, then adversarially verified every candidate finding before reporting. 9 findings were CONFIRMED and 1 was marked PLAUSIBLE.
Two fixes needed before this is safe on a clean Windows machine:
- 1 (L230) — downloaded scripts get a Zone.Identifier ADS that blocks execution under the default
RemoteSignedpolicy; addUnblock-FileafterMove-Item. - 2 (L253) —
Invoke-RestMethodsilently succeeds on non-200 2xx responses and round-trips the JSON through PowerShell's object model; switch toInvoke-WebRequestwith an explicit 200 check and write.Contentverbatim.
fivetran-paulharper
left a comment
There was a problem hiding this comment.
Follow-up review (Claude, claude-sonnet-4-6)
All 9 CONFIRMED findings from the previous review are addressed in this revision. Quick status:
| Finding | Status |
|---|---|
| Zone.Identifier ADS blocks manager script | ✅ Fixed — Unblock-File added after Move-Item |
Invoke-RestMethod 2xx silent success / null config |
✅ Fixed — switched to Invoke-WebRequest with explicit 200 check |
| JSON round-trip precision loss | ✅ Fixed — $response.Content written verbatim |
| Temp filename race condition | ✅ Fixed — now uses $PID suffix |
Test-Path matches files not just directories |
✅ Fixed — Test-Path -PathType Container |
Reinstall AccessDenied on config write |
✅ Fixed — catch [UnauthorizedAccessException] with friendly message |
Config ACL grants ExecuteFile on JSON file |
✅ Fixed — changed to Read,Write |
| Config readability not checked | ✅ Fixed — try/catch on Get-Content |
$env:TOKEN derived mid-script leaked to subprocesses |
✅ Fixed — $derivedToken local var + try/finally cleanup |
| Administrator check is a warning not a hard exit | ℹ️ Unchanged — appears intentional |
One residual note: user-supplied $env:TOKEN (set by the caller before running the script) is still present in the environment when docker --version and docker info are spawned inside Test-DockerVersion. The try/finally cleanup clears it before the script exits, but it is briefly visible to those child processes during pre-flight. This is inherent to the caller having already exported it, so it is low severity — just worth being aware of.
Adding new Windows install script
Tested on a Windows machine:
Run the manager script to check the status: