Skip to content

fix(#828): preserve GitHub error body and status in mint errors - #856

Open
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/828-mint-error-propagation
Open

fix(#828): preserve GitHub error body and status in mint errors#856
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/828-mint-error-propagation

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

Summary

Preserves GitHub's error response body and status code in mint token failures instead of discarding the body and collapsing all upstream errors to 502. This makes mint failures debuggable from GitHub Actions logs (where maintainers look first) rather than requiring direct access to Cloud Run logs.

Related Issue

Fixes #828

Changes

  • Add GitHubAPIError type in internal/mintcore/github.go that preserves the upstream HTTP status code and response body
  • Replace io.Copy(io.Discard, ...) with readErrorBody() in all five GitHub API call sites (FindInstallation, FindOrgInstallation, GetOrgVariable, createInstallationTokenWithPermissions, CreateInstallationToken)
  • Add upstreamStatus() helper in handler.go that forwards 4xx from GitHub as-is (non-retriable client errors) and returns 502 only for 5xx/network errors
  • Sync embed copies (github.go.embed, handler.go.embed) for GCF deployment

Testing

  • TestCreateInstallationToken_NonCreatedStatus_IncludesBody — verifies 422 body is preserved
  • TestCreateInstallationToken_5xxStatus_IncludesBody — verifies 500 body is preserved
  • TestFindInstallation_ErrorIncludesBody — verifies 403 body is preserved
  • TestHandler_GitHubTokenCreation422_ForwardedAsIs — verifies handler forwards 422 instead of 502
  • TestHandler_GitHub5xx_Returns502 — verifies 5xx still returns 502
  • TestHandler_InstallationNotFound — updated to expect 404 (was 502)
  • Full mintcore test suite passes with race detection (83.6% coverage)

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • Embed copies synced for GCF deployment

Closes #828

Post-script verification

  • Branch is not main/master (agent/828-mint-error-propagation)
  • Secret scan passed (gitleaks — c887fc47c59d8b87b5282ca44044d2933d30df41..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

GitHub API error responses were discarded with io.Copy(io.Discard, ...)
and all upstream failures were collapsed to HTTP 502 (Bad Gateway),
making mint token failures impossible to debug from GitHub Actions logs.

- Add GitHubAPIError type that preserves the upstream status code and
  response body from GitHub API calls
- Read the response body (size-limited to 4096 bytes) instead of
  discarding it, so GitHub's error message appears in mint service logs
- Forward GitHub 4xx status codes to the client instead of collapsing
  them to 502, allowing clients to distinguish retriable (5xx) from
  non-retriable (4xx) errors
- Apply consistently to FindInstallation, FindOrgInstallation,
  GetOrgVariable, createInstallationTokenWithPermissions, and
  CreateInstallationToken
- Sync embed copies for GCF deployment

Closes #828
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:05 AM UTC · Completed 9:20 AM UTC
Commit: 9ea2db3 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [api-contract] internal/mintcore/handler.goupstreamStatus forwards any GitHub 4xx status code to the mint client, including 401. The mint handler already uses 401 for OIDC authentication failures (body {"error":"authentication failed"}). If GitHub returns 401 (e.g., expired App JWT), the client receives 401 with body {"error":"mint failed"} — distinguishable by body text but ambiguous by status code alone. In practice, GitHub 401 on a fresh App JWT is extremely rare (JWTs are generated per-request with 10-minute expiry).
    Remediation: Consider clamping GitHub 401 to 502 in upstreamStatus since the client cannot fix the mint's internal JWT.

  • [breaking-api] internal/mintcore/handler.go — HTTP status code behavior changes from always-502 to forwarded-4xx for GitHub API errors. This is the intended fix per issue mint: 422 from GitHub is swallowed into an opaque generic 502 #828 and makes status codes semantically correct (4xx = non-retriable client error, 502 = retriable upstream error). The primary consumer (mint-token action) uses --retry-all-errors and is unaffected. Note this behavioral change in release notes.

  • [documentation] internal/mintcore/github.goGitHubAPIError has a godoc comment but does not document field population guarantees (e.g., Body may be empty if the response body read fails or GitHub returns no body).

Previous run

Review

Findings

Medium

  • [incomplete-application-of-pattern] internal/mintcore/handler.gomintTokenCrossOrg still hardcodes http.StatusBadGateway for errors from loadForeignAllowlist (line ~387), while mintToken was updated to use upstreamStatus(err). Cross-org mints still collapse all upstream GitHub failures to 502, even when GitHub returned a 4xx that should be forwarded. Additionally, fetchForeignAllowlist wraps the FindOrgInstallation error with %v instead of %w (line ~465), which breaks the errors.As chain needed by upstreamStatus. Consider applying upstreamStatus(err) in mintTokenCrossOrg and fixing the %v%w wrapping for consistency with the same-org path.

Low

  • [comment-accuracy] internal/mintcore/github.go:43 — The readErrorBody docstring claims it "drains any remainder" but the implementation only reads up to maxBytes via LimitReader and does not drain. No functional impact (resp.Body.Close() handles cleanup), but the comment is misleading.
  • [data-exposure] internal/mintcore/handler.go — Forwarding GitHub 4xx status codes to authenticated callers provides minor signal about upstream state (e.g., 404 reveals the target doesn't exist vs. a generic 502). Risk is minimal since callers are OIDC-authenticated and the response body is always the hardcoded string "mint failed". This is an intentional design choice per issue mint: 422 from GitHub is swallowed into an opaque generic 502 #828.
  • [partial-scope] Issue mint: 422 from GitHub is swallowed into an opaque generic 502 #828 identifies three compounding problems: (1) error body discarded, (2) status collapsed to 502, (3) blind retries in mint-token action. This PR addresses problems 1 and 2. Problem 3 (client-side retry logic on 4xx) is not addressed and no follow-up issue is linked.

Labels: PR fixes a confirmed defect in mint error handling (issue #828), matching the type/bug label convention used for bug fixes in this repo.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment type/bug Confirmed defect in existing behavior labels Aug 2, 2026
@guyoron1

guyoron1 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

/fs-fix

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 2, 2026

Copy link
Copy Markdown
Author

🤖 Finished Fix · ✅ Success · Started 10:31 AM UTC · Completed 10:38 AM UTC
Commit: 9ea2db3 · View workflow run →

- Use upstreamStatus(err) in mintTokenCrossOrg so cross-org mints
  forward GitHub 4xx instead of collapsing all errors to 502
- Change fetchForeignAllowlist's FindOrgInstallation wrapping from %v
  to %w so errors.As can find the GitHubAPIError through the chain
- Fix readErrorBody docstring that incorrectly claimed it drains
  remaining bytes
- Sync embed copies

Addresses review feedback on #856
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 1 (human-triggered)

Fixed 2 code issues and 1 docstring issue from the review. Applied upstreamStatus(err) in mintTokenCrossOrg and changed %v to %w in fetchForeignAllowlist so the GitHubAPIError chain is preserved for cross-org mints. Fixed readErrorBody docstring accuracy. All changes synced to embed copies. Disagreed with 2 informational findings (data-exposure is intentional per #828, partial-scope is a tracking note).

Fixed (4):

  1. incomplete-application-of-pattern: mintTokenCrossOrg hardcodes StatusBadGateway (internal/mintcore/handler.go): Changed mintTokenCrossOrg to use upstreamStatus(err) instead of http.StatusBadGateway, matching the pattern already applied in mintToken
  2. incomplete-application-of-pattern: fetchForeignAllowlist wraps error with %v breaking errors.As chain (internal/mintcore/handler.go): Changed FindOrgInstallation error wrapping in fetchForeignAllowlist from %v to %w so errors.As can traverse the chain and upstreamStatus can find the GitHubAPIError
  3. comment-accuracy: readErrorBody docstring claims it drains remainder (internal/mintcore/github.go): Removed 'drains any remainder, and' from the readErrorBody docstring since the implementation only reads up to maxBytes via LimitReader
  4. embed sync: handler.go.embed and github.go.embed (internal/dispatch/gcf/mintsrc/mintcore/handler.go.embed): Synced all three fixes to the GCF deployment embed copies (handler.go.embed and github.go.embed)

Disagreed (2):

  1. data-exposure: forwarding 4xx status codes reveals upstream state: The reviewer notes this is an intentional design choice per issue mint: 422 from GitHub is swallowed into an opaque generic 502 #828. The risk is minimal since callers are OIDC-authenticated and the response body is always the hardcoded string 'mint failed'. No code change needed.
  2. partial-scope: problem 3 (client-side retry on 4xx) not addressed: This is a scope observation, not a code fix request. The PR addresses problems 1 and 2 from issue mint: 422 from GitHub is swallowed into an opaque generic 502 #828 as stated. Problem 3 (client-side retry logic) is a separate concern for a follow-up issue.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:40 AM UTC · Completed 10:59 AM UTC
Commit: eac5e97 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Aug 2, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had any activity in the last month. It will be closed in 2 weeks if no further activity occurs. Remove the stale label to reset the inactivity timer.

@github-actions github-actions Bot added the stale label Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge stale type/bug Confirmed defect in existing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mint: 422 from GitHub is swallowed into an opaque generic 502

1 participant