Skip to content

Restore Go vendor mode and retry tool downloads - #172

Merged
gontzess merged 1 commit into
mainfrom
steve.gontzes/gf13-inc1-module-mode
Oct 22, 2026
Merged

gontzess merged 1 commit into
mainfrom
steve.gontzes/gf13-inc1-module-mode

Conversation

@gontzess

@gontzess gontzess commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Incident

Incident triage reported 13 failed release runs across two classes: vendored private-module resolution failures and transient toolchain download/stream errors.

An explicit GOFLAGS=-mod=readonly disables Go's automatic vendor mode. A caller with a complete vendor tree therefore attempts module resolution instead of using its vendored private dependencies. The job's proxy-only GOPROXY=https://proxy.golang.org also removes Go's normal direct fallback, exposing private-module 404s. Vendor mode and readonly mode both refuse required go.mod edits; forcing a module mode job-wide is not needed for that hygiene property.

Fix

  • Remove only the three job-level GOFLAGS, GOPROXY, and GOSUMDB assignments from goreleaser-binaries. The required whole-file inventory found no matching assignments in any other release job.
  • Keep GOTOOLCHAIN=local, locked tool versions/checksums, module gates, and the trusted gon build's separate step-local module/checksum settings unchanged.
  • Retry artifact downloads with curl --retry 5 --retry-delay 2 --retry-all-errors; PowerShell uses Invoke-WebRequest -MaximumRetryCount 5 -RetryIntervalSec 2. Hash-before-extract and the committed download origins remain unchanged.
  • Add four lines to the existing CI-owned no-self-reference/workflow test to reject workflow/job-level module-resolution overrides. No new CI job or checker framework is introduced.

Related fixes and merge order

#171 owns the separate Git credential-form fix and the root .golangci.yml readonly removal. Neither is duplicated here.

That root configuration is upstream of the fleet sync and pairs with the baton-admin template guard: #171 merges first, then the downstream template PR. This incident fix has no downstream-template dependency. This repository's module graph contains only public dependencies and no tracked vendor tree.

This branch was created from main and rebased onto 49d2c72e6150c82c8cae8e37fdb0dc5ce42aab79 after #171 merged; its incident patch is unchanged in range-diff. It is not stacked on an open feature branch. The incident change is limited to caller module-mode defaults, bounded installer retries, and their existing CI/fixture coverage.

Evidence

  • make test-scripts: PASS, including 28 tool-integrity tests (native Windows PowerShell fixture skipped locally).
  • GOTOOLCHAIN=local go test ./... and GOTOOLCHAIN=local go vet ./...: PASS.
  • make workflow-validate, Bash syntax check of install-tool.sh, and git diff --check: PASS.
  • Actual CI assertion smoke: 33 workflow/job environment mutations were rejected; normal and trusted step-local settings passed.
  • Real Go fixture smoke with network disabled: a synthetic private vendored dependency builds under default vendor mode; adding -mod=readonly bypasses vendoring and fails module resolution. go.mod remains unchanged.
  • Real curl fixture smoke: an empty-response stream failure is retried successfully with the new flags; removing --retry-all-errors fails immediately with exit 52 before extraction/PATH publication. Successful bytes still pass the fixture's committed checksum.
  • The native Windows harness now checks both retry parameters. Native Windows/PowerShell execution is unavailable locally; the existing Windows CI job exercises that harness.

Known full-lint limitation (pre-existing)

Full golangci-lint run --timeout=5m ./... with v2.14.0 exits 3 because gocritic/ruleguard references the absent tools/rules.go.

The comparison using main 49d2c72e's configuration also exits 3 with the same ruleguard init error: no file matching .../tools/rules.go. Go sources are unchanged from main. This is not introduced by the module-mode fix; no unrelated lint rules are changed here.

Scope and follow-ups

  • No second-source/mirror code, lock-file column, dist mirror, or hosted-toolcache fallback is added. A second source is banked only if stream errors persist after these retries.
  • No outer GoReleaser retry wrapper: it was considered and rejected because partially published releases can collide with immutable/versioned outputs.
  • A vendored-private-module canary belongs in a separate baton-github-test PR and a separately authorized live release sequence; it is not created or dispatched here.
  • No v4 tag change, merge, deployment, credential configuration, or live release is performed by this draft PR. Prepared fixtures and local smoke tests are not deployed-canary evidence.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit b136fe2d5d0f

General PR Review: Restore Go vendor mode and retry tool downloads

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base 49d2c72e6150.
Review mode: full
View review run

Review Summary

The PR removes the job-level GOFLAGS=-mod=readonly, GOPROXY=https://proxy.golang.org and GOSUMDB=sum.golang.org from goreleaser-binaries (.github/workflows/release.yaml:392-394). Go's defaults then apply: vendor mode when a vendor tree exists, the proxy.golang.org,direct fallback, and the same checksum database. The trusted gon build keeps its step-local values (.github/actions/locked-tools/action.yml:89-91, enforced by gon-source.sh:27), and verify-locked-go.sh still pins its own offline settings, so neither is affected. The PR also raises the installer retries: curl gets --retry 5 --retry-delay 2 --retry-all-errors, and pwsh Invoke-WebRequest gets 5 retries at a 2-second interval. The SHA256 check still runs before extraction. Docs and tests match the change: the curl stub asserts the flags, the pwsh harness checks both parameters, and the yq test now rejects these variables at workflow or job scope. I scanned the full PR diff for security and correctness. No repo-local criteria loaded, so only the base criteria were applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • New (medium confidence): .github/actions/locked-tools/install-tool.ps1:43. Invoke-WebRequest -MaximumRetryCount only retries on HTTP 304/400–599 responses, not on connection resets or stream failures. The Windows path therefore doesn't get the stream-error coverage that --retry-all-errors gives the curl path.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/locked-tools/install-tool.ps1`:
- Around line 43: Invoke-WebRequest -MaximumRetryCount only retries on HTTP 304/400-599 responses, not on transport or stream failures (connection reset, truncated body). To match the curl --retry-all-errors behavior, wrap the download in a bounded loop: up to 5 attempts, catch the exception, Start-Sleep -Seconds 2, rethrow after the last attempt. Keep the SHA256 check after the loop, and update the Windows test harness in scripts/test-tool-integrity.py to match.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@gontzess
gontzess marked this pull request as ready for review October 22, 2026 22:12
@gontzess
gontzess requested a review from a team October 22, 2026 22:12
try {
$archive = Join-Path $workdir 'artifact.zip'
Invoke-WebRequest -Uri $url -OutFile $archive -MaximumRetryCount 3
Invoke-WebRequest -Uri $url -OutFile $archive -MaximumRetryCount 5 -RetryIntervalSec 2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion (medium confidence): Invoke-WebRequest -MaximumRetryCount only retries on HTTP 304/400–599 responses, not on connection resets or mid-body stream failures. So the Windows path does not get the stream-error coverage that --retry-all-errors gives the curl path, which is the incident class this PR targets. If Windows stream errors were part of the 13 failures, wrap the call in a small bounded retry loop (e.g. 5 attempts, Start-Sleep 2).

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit d86c0485519c

General PR Review: Restore Go vendor mode and retry tool downloads

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base 49d2c72e6150.
Review mode: full
View review run

Review Summary

The PR removes the job-level GOFLAGS=-mod=readonly, GOPROXY=https://proxy.golang.org and GOSUMDB=sum.golang.org from goreleaser-binaries (.github/workflows/release.yaml:392-394). Go's defaults then apply: vendor mode when a vendor tree exists, the proxy.golang.org,direct fallback, and the same checksum database. The trusted gon build keeps its step-local values (.github/actions/locked-tools/action.yml:89-91, enforced by gon-source.sh:27), and verify-locked-go.sh still pins its own offline settings, so neither is affected. The PR also raises the installer retries: curl gets --retry 5 --retry-delay 2 --retry-all-errors, and pwsh Invoke-WebRequest gets 5 retries at a 2-second interval. The SHA256 check still runs before extraction. Docs and tests match the change: the curl stub asserts the flags, the pwsh harness checks both parameters, and the yq test now rejects these variables at workflow or job scope. I scanned the full PR diff for security and correctness. No repo-local criteria loaded, so only the base criteria were applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • New (medium confidence): .github/actions/locked-tools/install-tool.ps1:43. Invoke-WebRequest -MaximumRetryCount only retries on HTTP 304/400–599 responses, not on connection resets or stream failures. The Windows path therefore doesn't get the stream-error coverage that --retry-all-errors gives the curl path.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/locked-tools/install-tool.ps1`:
- Around line 43: Invoke-WebRequest -MaximumRetryCount only retries on HTTP 304/400-599 responses, not on transport or stream failures (connection reset, truncated body). To match the curl --retry-all-errors behavior, wrap the download in a bounded loop: up to 5 attempts, catch the exception, Start-Sleep -Seconds 2, rethrow after the last attempt. Keep the SHA256 check after the loop, and update the Windows test harness in scripts/test-tool-integrity.py to match.

Reviewed commit: b136fe2d5d0f

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found — see the full review report

@gontzess
gontzess force-pushed the steve.gontzes/gf13-inc1-module-mode branch from b136fe2 to d86c048 Compare October 22, 2026 22:14
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit d86c0485519c

General PR Review: Restore Go vendor mode and retry tool downloads

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base 49d2c72e6150.
Review mode: full
View review run

Review Summary

This PR removes the job-level GOFLAGS=-mod=readonly, GOPROXY=https://proxy.golang.org, and GOSUMDB=sum.golang.org from goreleaser-binaries in release.yaml. Go's automatic -mod=vendor selection and the caller's default proxy fallback now apply to caller builds again. The trusted gon build keeps its step-local settings (locked-tools/action.yml:88-91), and gon-source.sh:27 still enforces them, so that path is unaffected. Download retries go from 3 to 5 with a 2s delay: curl also gets --retry-all-errors, and PowerShell gets -RetryIntervalSec 2. Both changes have test coverage. The curl stub records $*, so the new assertion at test-tool-integrity.py:136 really checks the flags. The new yq-based check rejects GOFLAGS, GOPROXY, and GOSUMDB at workflow or job scope across all jobs.

I scanned the full PR diff for security and correctness. Hash-before-extract ordering and download origins are unchanged. No repo-local criteria loaded, so only the base criteria were applied. go.mod/go.sum are unchanged and nothing was dropped from the diff.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • Prior — still present (medium confidence): .github/actions/locked-tools/install-tool.ps1:43. PowerShell's Invoke-WebRequest -MaximumRetryCount retries only on HTTP 304 and 400–599 responses. It does not retry connection resets or failures in the middle of the response stream, which is one of the incident's failure classes. Windows installs therefore don't get the stream-error protection that --retry-all-errors gives the curl path. The docs at docs/release-workflow.md:291-293 say PowerShell only "uses its native retry count", so they don't overclaim. An open thread already covers this.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/locked-tools/install-tool.ps1`:
- Around line 43: Invoke-WebRequest -MaximumRetryCount only retries on HTTP 304/400-599 status codes, not on transport or stream errors such as connection resets or truncated bodies. To match the curl path's --retry-all-errors behavior, wrap the download in a bounded loop: up to 5 attempts, 2s sleep between attempts, catching any exception and deleting the partial $archive before retrying. Rethrow after the final attempt. Keep the SHA256 check after the loop, before extraction. Update the Windows harness in scripts/test-tool-integrity.py to match.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

General PR Review: Restore Go vendor mode and retry tool downloads

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base 49d2c72e6150.
Review mode: full
View review run

Review Summary

This PR removes the job-level GOFLAGS=-mod=readonly, GOPROXY=https://proxy.golang.org, and GOSUMDB=sum.golang.org from goreleaser-binaries in release.yaml. Go's automatic -mod=vendor selection and the caller's default proxy fallback now apply to caller builds again. The trusted gon build keeps its step-local settings (locked-tools/action.yml:88-91), and gon-source.sh:27 still enforces them, so that path is unaffected. Download retries go from 3 to 5 with a 2s delay: curl also gets --retry-all-errors, and PowerShell gets -RetryIntervalSec 2. Both changes have test coverage. The curl stub records $*, so the new assertion at test-tool-integrity.py:136 really checks the flags. The new yq-based check rejects GOFLAGS, GOPROXY, and GOSUMDB at workflow or job scope across all jobs.

I scanned the full PR diff for security and correctness. Hash-before-extract ordering and download origins are unchanged. No repo-local criteria loaded, so only the base criteria were applied. go.mod/go.sum are unchanged and nothing was dropped from the diff.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • Prior — still present (medium confidence): .github/actions/locked-tools/install-tool.ps1:43. PowerShell's Invoke-WebRequest -MaximumRetryCount retries only on HTTP 304 and 400–599 responses. It does not retry connection resets or failures in the middle of the response stream, which is one of the incident's failure classes. Windows installs therefore don't get the stream-error protection that --retry-all-errors gives the curl path. The docs at docs/release-workflow.md:291-293 say PowerShell only "uses its native retry count", so they don't overclaim. An open thread already covers this.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/locked-tools/install-tool.ps1`:
- Around line 43: Invoke-WebRequest -MaximumRetryCount only retries on HTTP 304/400-599 status codes, not on transport or stream errors such as connection resets or truncated bodies. To match the curl path's --retry-all-errors behavior, wrap the download in a bounded loop: up to 5 attempts, 2s sleep between attempts, catching any exception and deleting the partial $archive before retrying. Rethrow after the final attempt. Keep the SHA256 check after the loop, before extraction. Update the Windows harness in scripts/test-tool-integrity.py to match.

Reviewed commit: d86c0485519c

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found — see the full review report

@gontzess
gontzess merged commit 27d6d5c into main Oct 22, 2026
7 checks passed
@gontzess
gontzess deleted the steve.gontzes/gf13-inc1-module-mode branch October 22, 2026 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants