Skip to content

feat(#5833): add repo-level foreign allow-list for mint - #5945

Merged
ifireball merged 5 commits into
mainfrom
agent/5833-repo-level-foreign-allowlist
Aug 6, 2026
Merged

feat(#5833): add repo-level foreign allow-list for mint#5945
ifireball merged 5 commits into
mainfrom
agent/5833-repo-level-foreign-allowlist

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Add repo-level FULLSEND_FOREIGN_<ROLE>_REPOS variable support to the mint's foreign authorization system, enabling per-repo foreign grants with scope restricted to specific target repositories and intra-org cross-repo access for per-repo callers.

Changes

  • github.go: Add GetRepoVariable and ReadForeignAllowlistFromRepo for reading repo-level Actions variables via GET /repos/{owner}/{repo}/actions/variables/{name}
  • handler.go: Extend mintTokenCrossOrg with repo-level FOREIGN fallback (union semantics with org-level grants); add checkRepoForeignGrants, loadRepoForeignAllowlist, fetchRepoForeignAllowlist; add intra-org cross-repo authorization path for per-repo callers in the main handler flow
  • repos_scope.go: Relax validateReposScope to allow non-empty repos in foreign mint requests (repo-level FOREIGN grants are validated separately in mintTokenCrossOrg)
  • foreign.go: Add repoForeignCacheKey helper for distinct cache keys
  • ADR 0083: Document the repo-level foreign allow-list mechanism, building on ADR 0060 and ADR 0078
  • architecture.md: Update cross-org authorization and repos scope sections

Testing

  • TestHandler_RepoLevelForeignGrant_CrossOrg — cross-org request with specific repos authorized via repo-level FOREIGN variable
  • TestHandler_RepoLevelForeignGrant_Denied — unauthorized caller denied even with repo-level variable set
  • TestHandler_RepoLevelForeignGrant_UnionWithOrgLevel — org-level grant takes precedence for installation-wide requests
  • TestHandler_IntraOrgRepoForeignGrant — per-repo caller accessing another repo in same org via repo-level grant
  • TestHandler_RepoLevelForeignGrant_ScopeRestriction — repo-level grant on one repo does not authorize other repos
  • Updated TestHandler_CrossOrgNonEmptyReposDenied for relaxed validateReposScope
  • Unit tests for GetRepoVariable, ReadForeignAllowlistFromRepo, repoForeignCacheKey
  • All existing tests pass with race detector
  • Embed sync test passes

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • I wrote this contribution myself and can explain all changes in it

Closes #5833

Post-script verification

  • Branch is not main/master (agent/5833-repo-level-foreign-allowlist)
  • Secret scan passed (gitleaks — 78f836dd99749c3db896155ddb8acac422f51f03..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

Extend the mint's foreign authorization system to support
repo-level FULLSEND_FOREIGN_<ROLE>_REPOS variables alongside
the existing org-level variables (ADR 0060). This enables
per-repo foreign grants with scope restricted to specific
target repositories, and intra-org cross-repo access for
per-repo callers.

Changes:
- github.go: add GetRepoVariable and
  ReadForeignAllowlistFromRepo for reading repo-level Actions
  variables
- handler.go: extend mintTokenCrossOrg with repo-level
  fallback (union semantics), add checkRepoForeignGrants,
  loadRepoForeignAllowlist, fetchRepoForeignAllowlist; add
  intra-org cross-repo path for per-repo callers
- repos_scope.go: relax validateReposScope to allow non-empty
  repos in foreign mint requests (repo-level FOREIGN grants
  validated in mintTokenCrossOrg)
- foreign.go: add repoForeignCacheKey helper
- ADR 0083: document the repo-level foreign allow-list
  mechanism
- architecture.md: update cross-org and repos scope sections

Note: pre-commit could not run due to sandbox network
restrictions (cannot fetch origin). The post-script runs
pre-commit authoritatively on the runner.

Closes #5833
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner August 5, 2026 17:58
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Aug 5, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 5, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:59 PM UTC · Completed 6:13 PM UTC
Commit: e786bb0 · View workflow run →

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Site preview

Preview: https://e696e5ab-site.fullsend-ai.workers.dev

Commit: 0965996e4b2d8f1701be70d8b4069e434c667319

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.51007% with 35 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/cli/foreign.go 76.51% 20 Missing and 15 partials ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [authorization-boundary] internal/mintcore/handler.go:522 — In mintTokenCrossOrg, when repos is non-empty, only repo-level FOREIGN grants are checked; org-level FOREIGN grants are never consulted. This is the intended disjoint authorization boundary per ADR 0083. However, an operator who has set an org-level FULLSEND_FOREIGN_<role>_REPOS variable (granting a caller installation-wide access) will find that the same caller is DENIED when they request specific repos unless separate repo-level variables are also set on each target repo. The error message "foreign caller not authorized for target repos" does not guide the operator toward the repo-level variable fix.
    Remediation: Enhance the error message to include guidance, e.g. "foreign caller not authorized for target repos (repo-level FULLSEND_FOREIGN_<ROLE>_REPOS variable required on each target repo)".
Previous run

Review

Findings

Medium

  • [authorization-invariant] internal/mintcore/repos_scope.go:67validateReposScope now unconditionally returns nil for all foreign (cross-org) requests, including those with non-empty repos. The prior code was fail-closed (rejected non-empty repos at the validation layer itself). The new design splits authorization across two functions (validateReposScope + mintTokenCrossOrg) with a mandatory coupling documented in an INVARIANT comment at line 48. The current handler correctly enforces this coupling, and the concern is forward-looking: if a new call site treats a nil return as "fully authorized" without the follow-up mintTokenCrossOrg check, it would bypass repo-level foreign authorization entirely. This is a defense-in-depth observation, not a current vulnerability.
    Remediation: Consider returning a sentinel shape label (e.g., "foreign-repo-scoped") from validateReposScope for the foreign-with-non-empty-repos case that the caller must handle, making the invariant compiler-visible rather than comment-documented.

Low

  • [naming-consistency] internal/mintcore/handler.go:622checkRepoForeignGrants is called from both mintTokenCrossOrg (cross-org primary authorization, line 522) and the intra-org fallback path (line 343). The function performs the same check in both contexts, but a reader unfamiliar with the code cannot tell from the name alone that it serves two distinct authorization paths.
    Remediation: Consider adding a doc comment clarifying the dual use case (cross-org primary vs. intra-org fallback).

  • [cli-flag-patterns] internal/cli/foreign.go:244 — The --repo flag accepts two forms (owner/repo and bare name with --org). This is consistent with existing dual-form conventions in the codebase (e.g., parseOrgOrRepo in inference.go, the install command's <org-or-owner/repo> argument) and is well-documented in the command's long help text.

Previous run (2)

Review

Findings

Medium

  • [technical-inaccuracy] docs/architecture.md:199 — The Mint repos scope bullet says "foreign mints with specific repos require either an org-level grant (covers all repos) or per-repo FOREIGN grants on each requested repo." The code in mintTokenCrossOrg routes requests with non-empty repos exclusively to repo-level FOREIGN grants — org-level FOREIGN is never consulted for repo-scoped requests. ADR 0083's "Authorization boundary" section correctly documents the disjoint routing: "Org-level grant → Not consulted for repo-scoped requests." The "either" phrasing implies union semantics that the code does not implement.
    Remediation: Replace "require either an org-level grant (covers all repos) or per-repo FOREIGN grants on each requested repo" with "require per-repo FOREIGN grants on each requested repo (org-level grants are not consulted for repo-scoped requests)."

Low

  • [fragile-dispatch] internal/mintcore/handler.go:340 — The intra-org repo-level FOREIGN fallback uses strings.Contains(scopeErr.Error(), "per-repo mint requires repos") to detect the specific per-repo denial. If the error message in validateReposScope is reworded, this substring match will silently stop triggering the fallback, resulting in fail-closed behavior (denying the request rather than allowing the intra-org cross-repo grant).
    Remediation: Consider using a sentinel error instead of string matching, or add a test that verifies the error message substring.

  • [incomplete-documentation] docs/guides/dev/e2e-testing.md:124 — The guide demonstrates fullsend admin foreign list/allow commands without mentioning the new --repo flag for repo-level foreign grants.
    Remediation: Add a note or example explaining that the foreign commands also support --repo for per-repository grants, referencing ADR 0083.

  • [missing-cross-reference] docs/guides/infrastructure/mint-administration.md — The mint-administration guide does not reference the fullsend admin foreign commands or cross-org authorization setup.
    Remediation: Add a section or cross-reference to ADR 0060 and ADR 0083.

  • [cli-flag-patterns] internal/cli/foreign.go — The --repo flag accepts two forms (owner/repo and bare name with --org), introducing a new dual-form flag pattern not present in other CLI commands.
    Remediation: Consider documenting this pattern or simplifying to require only one form.

Previous run (3)

Review

Findings

Medium

  • [technical-inaccuracy] docs/architecture.md:196 — The architecture.md describes repo-level FOREIGN grants as having "union semantics alongside org-level grants," but the code implements mutually exclusive routing: mintTokenCrossOrg routes requests with non-empty repos exclusively to repo-level FOREIGN checks (org-level is not consulted), and requests with empty repos exclusively to org-level FOREIGN checks (repo-level is not consulted). ADR 0083 itself correctly states the authorization boundaries are disjoint: "Org-level grant → authorizes only installation-wide tokens. Not consulted for repo-scoped requests" and vice versa.
    Remediation: Replace "with union semantics alongside org-level grants" with phrasing matching ADR 0083's "Authorization boundary" section, e.g., "with disjoint authorization boundaries from org-level grants (repo-level for repo-scoped requests, org-level for installation-wide requests)."

Low

  • [authorization-bypass] internal/mintcore/handler.go:336 — The intra-org repo-level FOREIGN grant fallback (lines 336–348) clears scopeErr whenever checkRepoForeignGrants succeeds, regardless of the original denial reason. Currently functionally correct (only one per-repo denial path reaches here), but if validateReposScope gains additional per-repo denial reasons, this catch-all fallback would silently override them.
    Remediation: Guard the fallback more precisely by checking that the scopeErr is specifically about the per-repo cross-repo denial.

  • [privilege-escalation] internal/mintcore/repos_scope.go:57validateReposScope now returns nil for all foreign requests regardless of repos content. The authorization is deferred to mintTokenCrossOrg, creating a temporal coupling. Currently safe (single call site correctly dispatches), but a comment documenting the invariant would be prudent.
    Remediation: Add a comment on validateReposScope documenting that callers MUST invoke mintTokenCrossOrg for foreign requests with non-empty repos.

  • [missing-test] internal/mintcore/handler_test.go — No test verifies that an org-level FOREIGN grant does NOT authorize a cross-org request with specific repos when no repo-level grant exists. This would directly verify the disjoint authorization boundary from ADR 0083.
    Remediation: Add a test: caller authorized by org-level FOREIGN grant, requests repos=["target-repo"], no repo-level FOREIGN variable on target-repo → expect 403.

Previous run (4)

Review

Findings

Medium

  • [technical-inaccuracy] docs/architecture.md:196 — The architecture.md describes repo-level FOREIGN grants as having "union semantics alongside org-level grants," but the code implements mutually exclusive routing: mintTokenCrossOrg routes requests with non-empty repos exclusively to repo-level FOREIGN checks (org-level is not consulted), and requests with empty repos exclusively to org-level FOREIGN checks (repo-level is not consulted). ADR 0083 itself correctly states the authorization boundaries are disjoint: "Org-level grant → authorizes only installation-wide tokens. Not consulted for repo-scoped requests" and vice versa.
    Remediation: Replace "with union semantics alongside org-level grants" with phrasing matching ADR 0083's "Authorization boundary" section, e.g., "with disjoint authorization boundaries from org-level grants (repo-level for repo-scoped requests, org-level for installation-wide requests)."

Low

  • [authorization-bypass] internal/mintcore/handler.go:336 — The intra-org repo-level FOREIGN grant fallback (lines 336–348) clears scopeErr whenever checkRepoForeignGrants succeeds, regardless of the original denial reason. Currently functionally correct (only one per-repo denial path reaches here), but if validateReposScope gains additional per-repo denial reasons, this catch-all fallback would silently override them.
    Remediation: Guard the fallback more precisely by checking that the scopeErr is specifically about the per-repo cross-repo denial.

  • [privilege-escalation] internal/mintcore/repos_scope.go:57validateReposScope now returns nil for all foreign requests regardless of repos content. The authorization is deferred to mintTokenCrossOrg, creating a temporal coupling. Currently safe (single call site correctly dispatches), but a comment documenting the invariant would be prudent.
    Remediation: Add a comment on validateReposScope documenting that callers MUST invoke mintTokenCrossOrg for foreign requests with non-empty repos.

  • [missing-test] internal/mintcore/handler_test.go — No test verifies that an org-level FOREIGN grant does NOT authorize a cross-org request with specific repos when no repo-level grant exists. This would directly verify the disjoint authorization boundary from ADR 0083.
    Remediation: Add a test: caller authorized by org-level FOREIGN grant, requests repos=["target-repo"], no repo-level FOREIGN variable on target-repo → expect 403.

Previous run (5)

Review

Findings

Medium

  • [error-handling-gap] internal/mintcore/handler.go:521 — In mintTokenCrossOrg, when checkRepoForeignGrants fails (lines 519–524), the error is silently discarded. If the failure is an infrastructure error (GitHub API down, PEM read failure), the caller receives a generic "foreign caller not authorized" 403 with no server-side logging. The org-level loadForeignAllowlist error on line 511 is correctly surfaced as a 502 Bad Gateway, but repo-level infrastructure errors are swallowed. See also: [error-handling-gap] finding at handler.go:339.
    Remediation: Log the error from checkRepoForeignGrants before falling through to the denial, e.g., log.Printf("repo-level foreign grant check failed: %v", err).

  • [stale-constraint] docs/ADRs/0077-mint-repos-scope-hardening.md:40 — ADR 0077 states "Foreign (cross-org) requests require empty repos. Non-empty lists are denied." This is now partially superseded by ADR 0083, which allows non-empty repos with repo-level FOREIGN grants. The validateReposScope function no longer denies non-empty repos for foreign mints.
    Remediation: Add a short "Later note" annotation to ADR 0077's Consequences section linking to ADR 0083.

Low

  • [error-handling-gap] internal/mintcore/handler.go:339 — In the intra-org path, when checkRepoForeignGrants fails, the error is not logged. The handler falls through to return the original scopeErr. Infrastructure failures during repo-level lookups are invisible in logs.
    Remediation: Add logging for the repo-level foreign grant check failure.

  • [missing-test] internal/mintcore/handler_test.go — No test for the scenario where a per-repo caller requests multiple intra-org repos with only a subset having repo-level FOREIGN grants. The all-or-nothing semantics of checkRepoForeignGrants are tested indirectly via TestHandler_RepoLevelForeignGrant_ScopeRestriction, but a direct partial-authorization test for the intra-org path would strengthen coverage.
    Remediation: Add a test with repos=["authorized-repo", "unauthorized-repo"] verifying 403 for partial intra-org grants.

  • [error-handling-idiom] internal/mintcore/handler.go:614checkRepoForeignGrants uses %w for error wrapping while fetchRepoForeignAllowlist and the existing fetchForeignAllowlist use %v. Minor pattern inconsistency in the handler layer.
    Remediation: Use %v in checkRepoForeignGrants for consistency.

  • [naming-convention] internal/mintcore/foreign.go:52 — The doc comment on repoForeignCacheKey reads as a run-on ("target org + repo + role repo-level policy lookups"). Compare with foreignCacheKey which reads cleanly.
    Remediation: Reword to separate summary from detail.

  • [naming-convention] internal/mintcore/github.go:381GetRepoVariable reuses orgVariableResponse to decode the response. Functionally correct (same JSON shape) but the type name is misleading for repo-level variables.
    Remediation: Consider renaming to variableResponse.

  • [stale-doc] docs/ADRs/0060-cross-org-mint-authorization-via-org-variables.md:76 — ADR 0060 consequences state roles need organization_actions_variables: read. With ADR 0083, repo-level grants also need actions_variables: read. No annotation links to ADR 0083.
    Remediation: Add an annotation in ADR 0060 pointing to ADR 0083.


Labels: PR modifies mint authorization system (internal/mintcore/) with security-relevant changes to foreign grant authorization paths

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 component/mint Token mint and cross-boundary credentials security Security threat model and related concerns labels Aug 5, 2026
@ifireball

Copy link
Copy Markdown
Member

/fs-fix

In mintTokenCrossOrg, when specific repos are requested,
do not consult the org-level FULLSEND_FOREIGN_* variable
at all — authorize only via per-repo FOREIGN grants.

Org-level FOREIGN then effectively authorizes only
installation-wide tokens (empty / ["*"]).

Update ADR 0083 and tests (drop union-for-specific-repos).
Also fix the other review-agent findings.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 5:52 AM UTC · Completed 6:06 AM UTC
Commit: e786bb0 · View workflow run →

In mintTokenCrossOrg, repo-scoped requests now authorize exclusively
via per-repo FOREIGN grants — the org-level variable is not consulted.
Org-level FOREIGN effectively authorizes only installation-wide tokens.
This eliminates union semantics for specific-repos requests.

Also addresses review-agent findings:
- Log errors from checkRepoForeignGrants in both cross-org and intra-org paths
- Use %v instead of %w in checkRepoForeignGrants for handler-layer consistency
- Rename orgVariableResponse to variableResponse (used for both org and repo)
- Reword repoForeignCacheKey doc comment for clarity
- Add later-note annotations to ADR 0060 and ADR 0077 referencing ADR 0083
- Update ADR 0083 to document authorization boundary (no union semantics)
- Add TestHandler_IntraOrgRepoForeignGrant_PartialDenied for partial grants
- Update existing tests to not mock org-level endpoints for repo-scoped requests
- Rename UnionWithOrgLevel test to OrgLevelForInstallationWide

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

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 11 findings: 1 core architectural change (human instruction to separate org/repo FOREIGN scopes), ADR 0083 update, test updates, and 8 review-agent findings (error handling, naming, stale docs, missing test).

Fixed (11):

  1. separate org-level and repo-level FOREIGN scopes (human instruction) (internal/mintcore/handler.go): Restructured mintTokenCrossOrg so repo-scoped requests authorize exclusively via per-repo FOREIGN grants; org-level FOREIGN is only consulted for installation-wide tokens. Mirrors change in handler.go.embed.
  2. update ADR 0083 to drop union semantics (human instruction) (docs/ADRs/0083-repo-level-foreign-allow-list.md): Updated 'Cross-org with specific repos' section and replaced 'Union semantics' with 'Authorization boundary' documenting the separated scopes.
  3. update tests to drop union-for-specific-repos (human instruction) (internal/mintcore/handler_test.go): Removed org-level mock endpoints from repo-scoped tests (CrossOrg, Denied, ScopeRestriction). Renamed UnionWithOrgLevel to OrgLevelForInstallationWide. Adjusted token call counters.
  4. error-handling-gap at handler.go:521 (cross-org path) (internal/mintcore/handler.go): Error from checkRepoForeignGrants is now logged before returning 403. The restructured code logs via log.Printf before the denial.
  5. error-handling-gap at handler.go:339 (intra-org path) (internal/mintcore/handler.go): Added else branch to log checkRepoForeignGrants failure in the intra-org per-repo fallback path.
  6. error-handling-idiom at handler.go:614 (%w vs %v) (internal/mintcore/handler.go): Changed %w to %v in checkRepoForeignGrants error wrapping for consistency with fetchForeignAllowlist and fetchRepoForeignAllowlist.
  7. naming-convention at foreign.go:52 (doc comment) (internal/mintcore/foreign.go): Reworded repoForeignCacheKey doc comment to separate summary from detail, matching foreignCacheKey style.
  8. naming-convention at github.go:381 (orgVariableResponse) (internal/mintcore/github.go): Renamed orgVariableResponse to variableResponse since it's used for both org and repo variable endpoints. Updated all references in source, embed, and test files.
  9. stale-constraint in ADR 0077 (docs/ADRs/0077-mint-repos-scope-hardening.md): Added Later note annotation linking to ADR 0083 explaining that the foreign empty-repos constraint is relaxed for repo-level FOREIGN grants.
  10. stale-doc in ADR 0060 (docs/ADRs/0060-cross-org-mint-authorization-via-org-variables.md): Added Later note annotation about repo-level foreign grants and the additional actions_variables:read permission requirement.
  11. missing-test for partial intra-org repos (internal/mintcore/handler_test.go): Added TestHandler_IntraOrgRepoForeignGrant_PartialDenied verifying that a per-repo caller requesting [authorized-repo, unauthorized-repo] gets 403.

Tests: passed

Decision points
  • Restructured mintTokenCrossOrg to check repo-level first (for specific repos) rather than org-level first with repo-level fallback (alternatives: Keep union semantics with org-level precedence, Check both and require agreement; rationale: Human instruction explicitly requested exclusive per-repo authorization for repo-scoped requests, removing the union/fallback pattern)

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 6, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:07 AM UTC · Completed 6:22 AM UTC
Commit: 1e4e385 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@ifireball

Copy link
Copy Markdown
Member

/fs-fix

Add --repo to the foreign CLI for repo-level
FULLSEND_FOREIGN__REPOS management:

  • fullsend admin foreign allow --repo <org/repo> --role --caller <org/repo|org>
  • fullsend admin foreign list --repo <org/repo> [--role ]
  • fullsend admin foreign revoke --repo <org/repo> --role --caller <org/repo|org>

--repo should accept owner/repo (consistent with other
CLI commands) so --org can be omitted. Also allow
--org --repo as the split form.

Without --repo, keep existing org-level behavior.
Update docs/tests. Also fix the other review-agent
findings.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 6:28 AM UTC · Completed 6:44 AM UTC
Commit: 1e4e385 · View workflow run →

Add --repo flag to fullsend admin foreign {allow,list,revoke} for
repo-level FULLSEND_FOREIGN_<role>_REPOS variable management:

  fullsend admin foreign allow --repo owner/repo --role <role> --caller <caller>
  fullsend admin foreign list --repo owner/repo [--role <role>]
  fullsend admin foreign revoke --repo owner/repo --role <role> --caller <caller>

--repo accepts owner/repo (so --org can be omitted) or bare repo name
with --org. Without --repo, existing org-level behavior is preserved.

Review findings addressed:
- Fix "union semantics" wording in architecture.md (disjoint boundaries)
- Guard intra-org fallback more precisely (check per-repo denial message)
- Add INVARIANT comment on validateReposScope for foreign requests
- Add TestHandler_OrgLevelForeignDoesNotAuthorizeRepoScoped

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

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (human-triggered)

Added --repo flag to fullsend admin foreign CLI (allow/list/revoke) for repo-level FULLSEND_FOREIGN__REPOS management. Addressed 4 review-agent findings: fixed architecture.md union semantics wording, added precise guard on intra-org fallback, added INVARIANT comment on validateReposScope, and added missing test for disjoint authorization boundary.

Fixed (6):

  1. add --repo to foreign CLI for repo-level variable management (internal/cli/foreign.go): Added --repo flag to allow, list, revoke subcommands. Accepts owner/repo (so --org can be omitted) or bare repo name with --org. Without --repo, existing org-level behavior is preserved. Added resolveForeignTarget helper, loadRepoForeignAllowlist, listRepoForeign, and foreignTarget type.
  2. technical-inaccuracy: union semantics wording in architecture.md (docs/architecture.md): Replaced 'with union semantics alongside org-level grants' with 'with disjoint authorization boundaries from org-level grants' to match ADR 0083.
  3. authorization-bypass: intra-org fallback clears scopeErr too broadly (internal/mintcore/handler.go): Added strings.Contains guard so the fallback only overrides the specific per-repo cross-repo denial. Applied same change to handler.go.embed.
  4. privilege-escalation: missing invariant comment on validateReposScope (internal/mintcore/repos_scope.go): Added INVARIANT comment documenting that callers MUST invoke mintTokenCrossOrg for foreign requests with non-empty repos. Applied same change to repos_scope.go.embed.
  5. missing-test: org-level FOREIGN should not authorize repo-scoped requests (internal/mintcore/handler_test.go): Added TestHandler_OrgLevelForeignDoesNotAuthorizeRepoScoped verifying disjoint authorization boundary from ADR 0083.
  6. add CLI tests for --repo flag (internal/cli/foreign_test.go): Added 11 new tests covering resolveForeignTarget, repo-level allow/list/revoke, split form, append, empty delete, and org-level backward compatibility.

Tests: passed

Decision points
  • Chose resolveForeignTarget helper over per-subcommand flag parsing (alternatives: Duplicate resolution logic in each subcommand, Use cobra PersistentPreRunE for validation; rationale: Single helper centralizes org/repo resolution, reduces duplication, and keeps subcommand RunE focused on business logic)
  • Used strings.Contains for per-repo denial guard instead of sentinel error type (alternatives: Define a typed error for per-repo denial, Export the error string as a constant; rationale: Matches existing error handling pattern in repos_scope.go (plain fmt.Errorf). Minimal change to address the concern about catch-all fallback)

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 6, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:45 AM UTC · Completed 7:04 AM UTC
Commit: 67f301a · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@ifireball

Copy link
Copy Markdown
Member

/fs-fix fix the review findings

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 7:22 AM UTC · Completed 7:34 AM UTC
Commit: 67f301a · View workflow run →

- Fix technical-inaccuracy in docs/architecture.md: replace "either"
  phrasing with accurate disjoint routing description (org-level grants
  are not consulted for repo-scoped requests)
- Replace fragile string matching in handler.go with sentinel error
  (errPerRepoCrossRepo) and errors.Is check for per-repo cross-repo
  denial dispatch
- Add --repo flag documentation to e2e-testing.md foreign grant examples
- Add cross-org authorization ADR cross-references to mint-administration.md
- Add sentinel error test (TestValidateReposScope_PerRepoSentinel)
- Keep embed copies in sync

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

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (human-triggered)

Addressed 4 of 5 review findings. Fixed technical-inaccuracy in architecture.md, replaced fragile string matching with sentinel error in handler.go/repos_scope.go, added --repo documentation to e2e-testing.md, added cross-references to mint-administration.md. Disagreed with cli-flag-patterns finding (already documented in CLI help).

Fixed (4):

  1. technical-inaccuracy: docs/architecture.md repos scope phrasing implies union semantics (docs/architecture.md): Replaced 'require either an org-level grant (covers all repos) or per-repo FOREIGN grants' with 'require per-repo FOREIGN grants on each requested repo (org-level grants are not consulted for repo-scoped requests)' to match the disjoint routing in mintTokenCrossOrg
  2. fragile-dispatch: handler.go uses strings.Contains for per-repo cross-repo denial detection (internal/mintcore/handler.go): Introduced errPerRepoCrossRepo sentinel error in repos_scope.go and replaced strings.Contains with errors.Is in handler.go. Added TestValidateReposScope_PerRepoSentinel test to verify sentinel behavior. Updated embed copies to stay in sync.
  3. incomplete-documentation: e2e-testing.md missing --repo flag for repo-level foreign grants (docs/guides/dev/e2e-testing.md): Added --repo flag examples for repo-level foreign grants with allow and list commands, plus cross-reference to ADR 0083
  4. missing-cross-reference: mint-administration.md has no reference to foreign grant commands or cross-org authorization (docs/guides/infrastructure/mint-administration.md): Added cross-references to ADR 0060 (org-level FOREIGN authorization) and ADR 0083 (repo-level foreign grants) in the See Also section

Disagreed (1):

  1. cli-flag-patterns: --repo flag accepts two forms (owner/repo and bare name with --org): The dual-form pattern is already documented in the command's help text (foreign.go lines 22-25 describe both forms with examples). Additional documentation would be out of scope for this PR. The dual-form is a usability feature — owner/repo avoids requiring --org, while bare name + --org provides consistency with existing commands.

Tests: passed

Decision points
  • Chose sentinel error over adding a test for the substring match (alternatives: Add test verifying error message substring, Use error type assertion; rationale: Sentinel error with errors.Is is idiomatic Go, eliminates the fragility entirely rather than just testing it, and matches the reviewer's suggestion)

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 6, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:35 AM UTC · Completed 7:50 AM UTC
Commit: bad02f9 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@ifireball

Copy link
Copy Markdown
Member

/fs-fix

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 7:59 AM UTC · Completed 8:09 AM UTC
Commit: bad02f9 · View workflow run →

Return reposScopeShapeForeignRepoScoped from validateReposScope when a
foreign request carries non-empty repos, making the repo-level FOREIGN
grant authorization requirement visible in the return value rather than
relying solely on the INVARIANT comment. Add doc comment to
checkRepoForeignGrants clarifying its dual use (cross-org primary
authorization and intra-org per-repo fallback).

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

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 4 (human-triggered)

Addressed 2 of 3 review findings. Added reposScopeShapeForeignRepoScoped sentinel shape for foreign+non-empty repos (making the authorization invariant value-visible rather than comment-only) and expanded checkRepoForeignGrants doc comment to clarify dual use. The cli-flag-patterns finding was informational with no remediation suggested.

Fixed (2):

  1. authorization-invariant: validateReposScope returns nil unconditionally for foreign requests (internal/mintcore/repos_scope.go): Added reposScopeShapeForeignRepoScoped constant and return it from validateReposScope when isTargetForeign && len(repos) > 0. This makes the repo-level FOREIGN grant authorization requirement visible in the return value rather than relying solely on a comment-documented INVARIANT. Updated doc comments, log messages, embed copies, and test expectations.
  2. naming-consistency: checkRepoForeignGrants serves two authorization paths without doc clarity (internal/mintcore/handler.go): Expanded doc comment on checkRepoForeignGrants to document both authorization paths: cross-org primary (from mintTokenCrossOrg) and intra-org fallback (from main handler on errPerRepoCrossRepo). Updated embed copy.

Disagreed (1):

  1. cli-flag-patterns: --repo flag accepts two forms (owner/repo and bare name with --org): The reviewer noted this is consistent with existing dual-form conventions in the codebase and is well-documented in the command's help text. No remediation was suggested. No action needed.

Tests: passed

Decision points
  • Chose to return a shape label string rather than changing the return type to a struct (alternatives: Return a struct with an explicit RequiresFollowUp field, Return a new error type that must be handled; rationale: The shape label approach matches the existing pattern (fullsend-any, enrolled-fullsend, enrolled-pair) and addresses the reviewer's specific suggestion without changing the function signature. The sentinel shape is self-documenting via the constant name and doc comment.)

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 6, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:10 AM UTC · Completed 8:23 AM UTC
Commit: 0965996 · View workflow run →

@ifireball
ifireball added this pull request to the merge queue Aug 6, 2026
Merged via the queue into main with commit 99c5c1d Aug 6, 2026
16 checks passed
@ifireball
ifireball deleted the agent/5833-repo-level-foreign-allowlist branch August 6, 2026 09:21
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 6, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 9:24 AM UTC · Completed 9:44 AM UTC
Commit: 0965996 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #5945 — repo-level foreign allow-list for mint

Timeline

Time (UTC) Event Actor
Aug 5, 13:21 Triage on issue #5833 triage agent
Aug 5, 17:34 Code agent run (31030650727) code agent
Aug 5, 17:58 PR #5945 opened (20 files, +1739/−107) code agent
Aug 5, 18:13 Review round 1: 5 findings (1 medium, 4 low) — error handling, naming review agent
Aug 6, 05:51 /fs-fix: separate org-level and repo-level authorization scopes (disjoint, not union) ifireball
Aug 6, 06:02 Fix 1: architectural change applied fix agent
Aug 6, 06:21 Review round 2: 3 findings (1 medium, 2 low) review agent
Aug 6, 06:27 /fs-fix: add --repo flag to foreign CLI ifireball
Aug 6, 06:44 Fix 2: --repo flag + review findings addressed fix agent
Aug 6, 07:04 Review round 3: 2 findings (1 medium, 1 low) review agent
Aug 6, 07:21 /fs-fix: fix the review findings ifireball
Aug 6, 07:34 Fix 3: addressed 4 of 5 review findings fix agent
Aug 6, 07:50 Review round 4: 3 findings (1 medium, 2 low) review agent
Aug 6, 07:58 /fs-fix ifireball
Aug 6, 08:06 Fix 4: sentinel shape + doc comments fix agent
Aug 6, 09:07 Approved ifireball
Aug 6, 09:21 Merged merge queue

Total: 14 agent runs dispatched (1 triage, 1 code, 6 review, 4 fix, 1 retro in-progress, 1 review cancelled as duplicate). 4 fix iterations, of which 2 were human-directed substantive changes and 2 were human-shepherded review finding fixes.

Key finding: review agent missed authorization boundary design

The review agent's round 1 found valid but surface-level issues (error handling gaps, naming inconsistencies). It did not evaluate the fundamental design question: should org-level and repo-level FOREIGN authorization use union semantics (consult both) or disjoint scopes (repo-level exclusively for repo-scoped requests)? The initial code implemented union semantics, which would let an org-level grant (intended for installation-wide access) implicitly authorize narrowly-scoped repo requests — a violation of least privilege on a security-labeled PR.

The human (ifireball) identified this ~12 hours later and directed the architectural fix. Notably, once the fix was applied, the review agent correctly flagged stale documentation that still referenced union semantics — showing it can distinguish the two models but only when comparing docs to code, not when evaluating the design itself.

This is evidence for #898 ("Review agent misses security-critical findings on large architectural PRs"). Issue #898 identifies three remaining gaps in the security sub-agent after the multi-sub-agent architecture was implemented. This PR reveals a fourth: the security sub-agent does not evaluate how authorization layers compose (whether multiple auth policies interact to create emergent privilege not visible in any single check). The existing remaining gaps focus on individual permission checks (fail-open detection, permission manifest guidance, large-PR prioritization) but not on composition semantics.

Evidence for other existing issues

  • #1582 (review agent should catch all findings in first pass): 13 findings across 4 review rounds. While some were on genuinely new code from fix iterations, the fragile-dispatch finding on handler.go (flagged in round 3) existed in the initial commit — evidence of incomplete first-pass coverage.
  • #1362 (filter bot-triggered label events): the ready-for-review label applied simultaneously with PR open triggered a duplicate review run (31032522087), which was cancelled.

Autonomy assessment

The review agent correctly applied requires-manual-review and security labels, demonstrating good triage judgment. Its mechanical correctness findings (error handling, naming, documentation accuracy) were valid and would have been sufficient for a non-security-critical PR. However, for authorization model design decisions, human review remains essential. The requires-manual-review label correctly gated this PR, and the human reviewer caught the critical design issue the agent missed. This class of PR — security-labeled changes to authorization logic — is not yet a candidate for increased agent autonomy.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/mint Token mint and cross-boundary credentials ready-for-review Triggers review agent dispatch requires-manual-review Review requires human judgment security Security threat model and related concerns

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mint: add a repo-level foreign allow-list for controlled inter-org and intra-org cross-repo access

1 participant