fix: address all High and Medium follow-up review items#22
Merged
Conversation
c4po
force-pushed
the
chore/medium-priority-fixes
branch
3 times, most recently
from
May 5, 2026 21:20
814c5b3 to
acea145
Compare
There was a problem hiding this comment.
Pull request overview
This PR bundles follow-up items from the latest review by improving wrapper robustness (constraint parsing, signal/exit-code behavior, env isolation), strengthening and de-duplicating release download/caching logic, and updating docs/tests/CI to match the new behavior.
Changes:
- Wrapper: tolerate parent-directory
.tfparse errors, forward additional Unix signals, map signaled exits to128+signum, and stripTF_DEMUX_*env vars before exec’ing Terraform. - Release client: add a 5-minute local TTL cache for the parsed index, fix zip size handling, remove partially-installed binaries on chmod failure, and serialize downloads with per-binary
flock. - Tooling/polish: add targeted regression tests, introduce buffered logging helper, update README/test script, and bump Go version in module/workflows.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
test.sh |
Uses TF_DEMUX_CACHE_HOME for integration tests to avoid macOS cache pollution. |
README.md |
Fixes typos and documents TF_DEMUX_CACHE_HOME cache override. |
internal/wrapper/wrapper.go |
Implements cache override, tolerant constraint loading, signal forwarding via build tags, exit-code normalization, and env filtering. |
internal/wrapper/wrapper_test.go |
Adds regression coverage for tolerant parent parse errors, env filtering, and cache override. |
internal/wrapper/signals_unix.go |
Adds SIGHUP/SIGQUIT forwarding on Unix. |
internal/wrapper/signals_windows.go |
Defines Windows signal-forwarding set. |
internal/wrapper/exitcode_unix.go |
Returns 128+signum when Terraform is killed by a signal. |
internal/wrapper/exitcode_windows.go |
Keeps Windows exit-code behavior portable. |
internal/wrapper/checkargs.go |
Moves state-command version constraints to package-level pre-parsed constants. |
internal/wrapper/checkargs_test.go |
Updates tests to reuse shared mustVersion helper. |
internal/wrapper/logging.go |
Introduces SetupLogging to buffer logs until flushed on error. |
internal/wrapper/logging_test.go |
Adds tests for buffered vs verbose logging behavior. |
internal/releaseapi/client.go |
Adds local index TTL caching, build selection helper, zip size fix, chmod cleanup, and per-binary download lock. |
internal/releaseapi/client_test.go |
Refactors server fixtures and adds TTL cache test. |
cmd/terraform-demux/main.go |
Switches to wrapper.SetupLogging and uses flush-on-error flow. |
go.mod |
Bumps Go directive and adds gofrs/flock. |
go.sum |
Updates sums for newly added/updated dependencies. |
.goreleaser.yaml |
Injects release tag into main.version via ldflags. |
.github/workflows/release.yaml |
Bumps Go version for release workflow. |
.github/workflows/ci.yaml |
Expands unit test OS matrix and bumps Go version; adds integration job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
(Updated to fold the code-quality review's High/Medium findings into this PR.) High - H-A: getTerraformVersionConstraints tolerates parse errors in parent dirs. - H1: client_test.go consolidates "stand up a fake release server" into a single serveRelease(t, version, zipBytes, shasumsBody) helper shared by all four download-related tests, plus a tiny newTestClient fixture for the happy-path cases. - H2: getTerraformVersionConstraints split into a thin walk loop + loadConstraintsAt helper that returns (constraints, found, err). - H3: DownloadRelease's match-build + nil-version-fallback logic extracted into findBuild(r, goos, arch) (Build, error). Medium - M-A: Forward SIGHUP / SIGQUIT (build-tagged forwardedSignals). - M-B: 128+signum on signaled child (build-tagged exitCodeFromError). - M-C: zip.NewReader uses zipFile.Stat().Size() instead of response.ContentLength. - M-D: Cached binary is removed if Chmod fails post-write. - M-E: Per-binary flock (gofrs/flock) serializes parallel downloads. - M-F: TF_DEMUX_CACHE_HOME env override; test.sh uses it instead of XDG_CACHE_HOME. - M-G: Strip TF_DEMUX_* from the env passed to the child terraform. - M-H: 5-minute local TTL on the parsed release index. - M1: Buffered-log-replay logic moved out of main.go into wrapper.SetupLogging(verbose, errSink) -> flush, with tests. - M2: readIndexCache logs on corrupt-cache JSON unmarshal failure instead of silently re-fetching. - M3: downloadBuild's unzip+install loop extracted into extractTerraformBinary; checksum and zip-reader setup also pulled into helpers, leaving downloadBuild as a linear five-step recipe. - M4: Dedup mustVer/mustVersion test helpers (kept mustVersion). - M5: GoDoc on package, ReleaseIndex, Release (incl. clarifying that Shasums is a *filename*, not a sum), Build, Client, NewClient, ListReleases, DownloadRelease, ShaSumsURL. Polish - L7: Renamed DownloadRelease's `os` parameter to `goos` (was shadowing the os package). - L12: writeIndexCache uses atomic.WriteFile. - L13: Documented TF_DEMUX_CACHE_HOME in README. - L-D: Bump go.mod's go directive and CI go-version to 1.24. - L-F: README typos fixed. Tests - New: TestSetupLogging_BuffersUntilFlush, TestSetupLogging_VerboseStreamsImmediately, parent-walk parse error tolerance, env filter, cache override, index TTL.
c4po
force-pushed
the
chore/medium-priority-fixes
branch
from
May 5, 2026 21:29
acea145 to
7b208ad
Compare
russtaylor
approved these changes
May 5, 2026
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
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.
Bundles the remaining High and Medium items from the latest review, plus the README typos and Go-version bump.
High
getTerraformVersionConstraintsno longer fails on parse errors in parent directories. A broken.tfin~/scratch/foopreviously bricked the wrapper everywhere under~; now only the user's actual cwd is a hard failure, ancestors get logged and walked past.Medium
SIGHUP/SIGQUITon Unix via a build-taggedforwardedSignalsslice — withoutSIGHUP, a closing terminal would orphan the child terraform process.128 + signumwhen the child is killed by a signal (shell/terraform convention), instead of-1fromexec.ExitError.ExitCode(). Implemented as a build-taggedexitCodeFromErrorso Windows stays portable.zip.NewReadernow reads size fromzipFile.Stat(), notresponse.ContentLength(which is-1for chunked responses).os.Chmodfails afteratomic.WriteFilesucceeds, the partial file is removed so the next invocation re-downloads instead of forever cache-hitting a non-executable binary.flockaround the download path serializes concurrent invocations so they don't both re-download the same archive. Usesgithub.com/gofrs/flock.TF_DEMUX_CACHE_HOMEenv var explicitly overrides the cache directory.os.UserCacheDir()ignoresXDG_CACHE_HOMEon macOS, so the integration suite has been writing into the real user cache;test.shnow setsTF_DEMUX_CACHE_HOMEinstead.TF_DEMUX_*env vars beforeexec'ing terraform. Otherwise a one-shotTF_DEMUX_ALLOW_STATE_COMMANDS=truewould silently extend through any nestedterraform-demuxinvocation.Polish
go.mod'sgodirective and both workflows'go-versionto1.24(1.21is past EOL).Compatability→Compatibility,recomment→recommend(x3), and the threeinstead \terraform state mv` commandbullets reworded to active voice withof` inserted where the verb was missing.Tests added
TestGetTerraformVersionConstraints_TolerantOfParentParseErrors— covers H-A regression.TestFilterDemuxEnv_StripsTFDemuxKeys— covers M-G.TestEnsureCacheDirectory_HonorsOverrideEnv— covers M-F.TestListReleases_UsesLocalIndexTTL— covers M-H by closing the upstream test server between two calls and asserting the second one still succeeds.Test plan
go test ./...cleangofmt -l .cleango vet ./...exit 0GOOS=windows go build ./...cleantestdata/terraform-pessimistic/