Skip to content

fix(#827): recover from 422 when config.yaml repos no longer exist - #857

Open
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/827-mint-stale-repo-recovery
Open

fix(#827): recover from 422 when config.yaml repos no longer exist#857
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/827-mint-stale-repo-recovery

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

Summary

When repos in config.yaml are deleted, transferred, or renamed from the org, the mint service's bulk installation-token request fails entirely — GitHub's POST /app/installations/{id}/access_tokens rejects the batch with 422 when any single repo name is invalid. This PR adds a retry-on-422 recovery path that validates each repo individually, filters out inaccessible ones, and retries the token request with only valid repos.

Changes

  • internal/mintcore/github.go: Add TokenCreationError typed error with HTTP status code; add ValidateRepoAccess function to check each repo via GET /repos/{org}/{repo}/installation; add DroppedRepos field to GrantedScope
  • internal/mintcore/handler.go: Add 422 retry logic in mintToken — on 422, validate repos individually, drop inaccessible ones, retry with valid subset; fall back to FindOrgInstallation when repos[0] is deleted (404 only — security errors propagate); add dropped_repos to mint response
  • Embedded copies synced to internal/dispatch/gcf/mintsrc/mintcore/

Key design decisions

  • Retry-on-422 rather than pre-validation: zero overhead in the happy path (all repos valid); validation only runs when GitHub rejects the batch
  • Only 404 triggers org-level fallback: cross-org installation mismatch and other security-relevant errors continue to propagate normally
  • Disabled-but-valid repos remain in scope: consistent with prior fix (fix(harness): exit gracefully when all repos are disabled fullsend-ai/fullsend#1833) — disabled repos need token access for unenrollment
  • All-invalid → clear 422 error: prevents minting an empty-scope token

Testing

  • TestHandler_StaleRepo422Recovery: mixed valid/invalid/disabled repos → token minted for valid+disabled, deleted repo reported as dropped
  • TestHandler_StaleRepo422Recovery_AllInvalid: all repos gone → clear 422 error
  • TestHandler_AllReposValid_NoRetry: happy path → single attempt, no retry overhead
  • TestHandler_StaleFirstRepo_FallbackToOrgInstallation: repos[0] deleted → org-level installation fallback → successful recovery
  • TestValidateRepoAccess*: unit tests for the validation function
  • TestCreateInstallationToken_Returns422AsTokenCreationError: typed error verification
  • All existing tests pass (including TestHandler_CrossOrgInstallationMismatch security check)

Closes #827

Post-script verification

  • Branch is not main/master (agent/827-mint-stale-repo-recovery)
  • 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

When repos listed in config.yaml are deleted, transferred, or renamed,
GitHub's POST /app/installations/{id}/access_tokens rejects the entire
batch with 422. This blocks all repos from being reconciled — even the
valid ones.

Add a retry-on-422 recovery path to mintToken:

1. If CreateInstallationToken returns 422, validate each repo
   individually via GET /repos/{org}/{repo}/installation.
2. Filter to only accessible repos and retry the token request.
3. Surface dropped repos in the response (dropped_repos field) so
   callers can log and alert on stale config entries.
4. If repos[0] itself is deleted (FindInstallation returns 404), fall
   back to FindOrgInstallation to obtain the installation ID.
5. If all repos are inaccessible, return a clear 422 error instead of
   an empty-scope token.

Security-relevant errors (e.g. cross-org installation mismatch) are
not affected — only 404 triggers the org-level fallback.

TokenCreationError is introduced as a typed error carrying the HTTP
status code, allowing callers to distinguish recoverable 422 from
other failures.

Closes #827
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:06 AM UTC · Completed 9:18 AM UTC
Commit: 530e79b · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [defense-in-depth] internal/mintcore/github.goValidateRepoAccess classifies repos as valid/invalid solely by HTTP status code without decoding the response body. Unlike FindInstallation, it does not verify Account.Login matches the expected org. Practical risk is limited: the function runs only in the 422 retry path after FindInstallation has already validated the installation with the cross-org check, and the result is used only to filter repos for the retry — not to establish authorization.

  • [naming-consistency] internal/mintcore/github.go — New error types installationLookupError and tokenCreationError use exported fields (StatusCode, Org, Repo) despite being unexported types. Existing unexported types in this package (mintError, foreignCacheEntry, foreignInflight) consistently use unexported fields.

  • [code-organization] internal/mintcore/github.go — The new error type declarations are placed between ReadForeignAllowlist and CreateInstallationToken, splitting the file's logical grouping. Existing types in this file are declared at the top before any functions.

  • [comment-style] internal/mintcore/github.go — The DroppedRepos field on GrantedScope uses an inline end-of-line comment. Existing GrantedScope fields have no comments; the Go convention for multi-word descriptions is a doc comment above the field.


Labels: PR fixes a bug in the mint token service's handling of stale repos

Previous run

Review

Findings

Medium

  • [pattern-violation] internal/mintcore/handler.go:368 — The FindInstallation 404 fallback uses strings.Contains(err.Error(), "status 404") to detect a 404. This is fragile and inconsistent with the TokenCreationError typed-error pattern introduced in the same PR for the 422 case. If FindInstallation's error message format changes, the fallback silently stops working. For consideration: introduce a typed error for FindInstallation non-200 responses mirroring the TokenCreationError pattern, and use errors.As for the 404 check.

  • [scope-vs-authorization] internal/mintcore/handler.go — Issue mint: undefined recovery behavior when a config.yaml repo no longer exists in the org #827 explicitly lists four open questions and states "Open question — no clear answer yet" about whether "drop and continue" is the correct behavior. This PR implements one option without a formal decision record resolving the alternatives. For consideration: record a decision (issue comment or PR description update) acknowledging this is the chosen approach and why alternatives were ruled out.

Low

  • [data-exposure] internal/mintcore/handler.go:41 — The dropped_repos field in the API response exposes which repos are inaccessible. Since the caller supplied these names and already has authenticated access to the org, the information leakage is minimal — but worth noting for awareness.

  • [scope-creep] internal/mintcore/handler.go:367 — The FindInstallation 404 fallback is a separate failure mode from the 422 recovery described in mint: undefined recovery behavior when a config.yaml repo no longer exists in the org #827. However, it is a necessary companion fix: without it, the 422 recovery would never be reached when repos[0] is the deleted repo (FindInstallation would fail first). The test TestHandler_StaleFirstRepo_FallbackToOrgInstallation validates this combined path.

  • [naming-convention] internal/mintcore/github.go:422TokenCreationError is exported but appears to be consumed only within the package via errors.As. The codebase convention for package-internal error types is unexported (e.g., mintError). Consider tokenCreationError for consistency.

  • [edge-case] internal/mintcore/github.go:492ValidateRepoAccess makes sequential HTTP requests (up to 500 per maxRepos). While 422 recovery is rare and 500 calls at ~200ms each (100s) stays within the 10-minute JWT window, this is worth noting for pathological cases.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label 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:39 AM UTC
Commit: 530e79b · View workflow run →

…eationError

Replace fragile strings.Contains(err.Error(), "status 404") check in the
FindInstallation 404 fallback with a typed installationLookupError and
errors.As, mirroring the TokenCreationError pattern for consistency.

Unexport TokenCreationError to tokenCreationError since it is only
consumed within the mintcore package, consistent with the mintError
convention.

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

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 2 of 6 review findings with code fixes. Introduced installationLookupError typed error for FindInstallation to replace fragile string matching (pattern-violation). Unexported TokenCreationError to tokenCreationError for naming consistency (naming-convention). The remaining 4 findings were awareness notes or process concerns with no code changes requested.

Fixed (2):

  1. pattern-violation: FindInstallation 404 fallback uses fragile string matching (internal/mintcore/handler.go): Introduced installationLookupError typed error for FindInstallation non-200 responses and replaced strings.Contains(err.Error(), "status 404") with errors.As check, mirroring the TokenCreationError pattern. Added test TestFindInstallation_Returns404AsInstallationLookupError to verify.
  2. naming-convention: TokenCreationError is exported but only used within the package (internal/mintcore/github.go): Unexported TokenCreationError to tokenCreationError, consistent with the mintError convention used elsewhere in the package. Updated all references in handler.go, github_test.go, and embed copies.

Disagreed (4):

  1. scope-vs-authorization: PR implements drop-and-continue without a formal decision record: Recording a decision record or updating issue comments is a process/documentation action, not a code fix. The fix agent's scope is limited to code changes addressing review findings. The PR description already documents the rationale and design decisions.
  2. data-exposure: dropped_repos field exposes which repos are inaccessible: Reviewer notes this is minimal risk and worth noting for awareness only — no code change was requested. The caller already supplied these names and has authenticated access to the org.
  3. scope-creep: FindInstallation 404 fallback is a separate failure mode: Reviewer explicitly validates this is a necessary companion fix and confirms the test covers the combined path. No change requested.
  4. edge-case: ValidateRepoAccess makes sequential HTTP requests up to 500: Reviewer notes this is worth noting for pathological cases — no code change was requested. The 422 recovery path is rare and stays within the JWT window.

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:41 AM UTC · Completed 10:54 AM UTC
Commit: 09b0102 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge type/bug Confirmed defect in existing behavior 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: undefined recovery behavior when a config.yaml repo no longer exists in the org

1 participant