fix(update): detect Windows GGA shims#300
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Windows-specific GGA installation detection so shim-based installs are treated as present during both availability checks and update checks (closing #177).
Changes:
- Extend Windows
ggaAvailable()detection to include~/bin/gga.ps1and~/bin/gga.exe(in addition to~/bin/gga). - Adjust update version detection to return an “unknown” installed version for Windows shim installs instead of treating GGA as missing.
- Add unit + integrated update-flow tests covering Windows shim presence.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/update/detect.go | Adds Windows shim filesystem detection and returns "unknown" when LookPath("gga") fails but a shim exists. |
| internal/update/check_test.go | Adds targeted tests for Windows shim detection and an integrated CheckFiltered assertion for GGA visibility/status. |
| internal/cli/run.go | Expands Windows GGA availability checks to include .ps1 and .exe shims under ~/bin. |
| internal/cli/gga_available_test.go | Adds Windows-specific tests ensuring ggaAvailable() detects gga.ps1 and gga.exe shims. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| cmd := execCommand(tool.DetectCmd[0], tool.DetectCmd[1:]...) | ||
|
|
||
| // Kill the subprocess when the context fires. We use a goroutine because | ||
| // the testable execCommand var returns a plain *exec.Cmd (not CommandContext). | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| select { | ||
| case <-detectCtx.Done(): | ||
| if cmd.Process != nil { | ||
| _ = cmd.Process.Kill() | ||
| } | ||
| case <-done: | ||
| } | ||
| }() |
There was a problem hiding this comment.
The Windows shim filename list ("gga.ps1", "gga.exe", "gga") is duplicated here and in cli.ggaAvailable(). This increases drift risk (one side updated without the other). Consider extracting the shim names / path-check logic into a shared helper or constant and reusing it from both call sites.
|
|
||
| results := CheckFiltered(context.Background(), "1.0.0", system.PlatformProfile{OS: "windows", PackageManager: "winget", Supported: true}, []string{"gga"}) | ||
| if len(results) != 1 { |
There was a problem hiding this comment.
The final assertion if results[0].Status == NotInstalled is redundant because the test already asserts results[0].Status != VersionUnknown / == VersionUnknown just above. Consider removing the redundant check to keep the test focused on a single expectation.
| results := CheckFiltered(context.Background(), "1.0.0", system.PlatformProfile{OS: "windows", PackageManager: "winget", Supported: true}, []string{"gga"}) | |
| if len(results) != 1 { |
|
Resolví el conflicto trayendo la rama al día con main y conservando ambos cambios en internal/update/check_test.go: el test de Windows shim para GGA y el test de parseVersionFromOutput(engram dev). Después corrí gofmt y go test ./... y volví a pushear la rama actualizada. |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The bugfix looks focused, but the validation state is stale and still shows a label check failure in older runs.
Please rebase or refresh the branch so CI re-runs cleanly with the current type:bug label, then this can go back into the quick-win queue.
🔗 Linked Issue
Closes #177
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New feature (non-breaking change that adds functionality)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes existing behavior)📝 Summary
LookPath("gga")fails.📂 Changes
internal/cli/run.goggaAvailable()now checks~/bin/gga.ps1,~/bin/gga.exe, and~/bin/gga.internal/update/detect.gounknowninstead of treating GGA as missing.internal/update/check_test.gointernal/cli/gga_available_test.gogga.ps1andgga.exeavailability tests.🧪 Test Plan
Unit Tests
go test ./...E2E Tests (Docker required)
go test ./...)cd e2e && ./docker-test.sh)🤖 Automated Checks
The following checks run automatically on this PR:
Closes/Fixes/Resolves #Nstatus:approvedtype:*Labeltype:*label must be appliedgo test ./...must passcd e2e && ./docker-test.shmust pass✅ Contributor Checklist
status:approvedtype:*label to this PRgo test ./...)cd e2e && ./docker-test.sh)Co-Authored-Bytrailers💬 Notes for Reviewers
This is a narrowly scoped Windows-only fix. The update path now has an integrated test proving GGA remains visible when only the Windows shim exists.