feat(mint): add e2e agent role for pool testing - #2407
Conversation
Register e2e in mintcore permissions and config.ValidRoles so mint add-role can bootstrap the e2e app before cross-org CI auth (fullsend-ai#2155). Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Site previewPreview: https://f7cab97d-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · |
Update TestValidRoles for the eighth role added for pool e2e minting. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 8:10 AM UTC · Completed 8:22 AM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsHigh
Medium
Low
Previous runReviewFindingsHigh
Medium
Low
Labels: PR adds a new privileged role to the token mint system for e2e pool testing Labels: PR adds a new role to the token mint system for e2e pool testing. Previous run (2)ReviewFindingsHigh
Medium
Low / Info
Previous runReviewFindingsHigh
Medium
Low / Info
Previous runReviewFindingsHigh
Medium
Low / Info
Previous run (3)ReviewFindingsHigh
Medium
Low / Info
Previous run (4)ReviewFindingsHigh
Medium
Low / Info
Previous runReviewFindingsHigh
Medium
Low / Info
Previous run (5)ReviewFindingsHigh
Medium
Low / Info
Previous run (6)ReviewFindingsHigh
Medium
Low / Info
|
Add AgentAppConfig case and AppPermissions fields so mint add-role --org creates an e2e GitHub App matching canonicalRolePermissions. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up on remaining findings (8ee2f1e): consumer-completeness ( consumer-completeness ( formatting-consistency — Keeping multi-line for the 11-key e2e map; can collapse to single-line in a follow-up if reviewers prefer strict parity. missing-architectural-documentation — Deferred to integration PR #2277 / ADR 0046 follow-up; this PR stays minimal (role registration + manifest alignment only). Prior thread replies cover permission-expansion, scope-ambiguity, and naming-alignment rationale. |
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 9:02 AM UTC · Completed 9:14 AM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 6-agent parallel review (Claude, Gemini, Codex)
2 new medium findings posted inline. 1 additional medium finding on a file outside the diff:
[medium] harnessesForRole("e2e") falls through to default, referencing non-existent harness
File: internal/layers/harnesswrappers.go:62
The default case returns []string{role}, so harnessesForRole("e2e") returns ["e2e"] — which looks for harness/e2e.yaml. No such scaffold file exists (only code, fix, prioritize, retro, review, triage). If e2e is ever added to an agent config that triggers harness wrapper installation, it will reference a missing file.
The PR description says "Pool tests are API/mint driven; no webhook events required" — so e2e should return nil like fullsend.
Suggested fix:
case "fullsend", "e2e":
return nilFlagged by 1/6 agents — verified against code
2 high findings (permission-expansion on mintcore/github.go:67, scope-ambiguity on config.go:94) already covered by existing review comments — skipped.
ralphbean
left a comment
There was a problem hiding this comment.
One non-blocking note: harnessesForRole("e2e") in internal/layers/harnesswrappers.go:56 falls through to the default case and returns []string{"e2e"}, which would try to reference a harness/e2e.yaml scaffold that doesn't exist. Since e2e is API/mint-driven with no harness, case "fullsend", "e2e": return nil would keep it safe. @waynesun09 flagged this too.
+1 on Wayne's two inline findings (cross-validation test, permission value assertions).
|
🤖 Finished Review · ✅ Success · Started 10:19 PM UTC · Completed 10:32 PM UTC |
| "retro": {"actions": "read", "contents": "read", "pull_requests": "write", "issues": "write", "metadata": "read"}, | ||
| "prioritize": {"contents": "read", "issues": "write", "organization_projects": "write", "metadata": "read"}, | ||
| "fullsend": {"actions": "write", "actions_variables": "read", "contents": "write", "pull_requests": "write", "workflows": "write", "metadata": "read"}, | ||
| "e2e": { |
There was a problem hiding this comment.
[high] permission-expansion
The new e2e role grants administration: write, organization_administration: write, secrets: write, and members: write — the most privileged GitHub App permissions available. No other role in the canonical permissions map requests organization_administration or secrets at write level. Combined, a minted e2e token could reconfigure repos, manage org settings, overwrite secrets, and modify org membership. The mint handler's ALLOWED_ROLES/ROLE_APP_IDS gating and exclusion from DefaultAgentRoles() are meaningful mitigating controls, but neither the code nor an ADR documents why each elevated permission is necessary, nor does the code enforce the pool-only constraint from issue #2155.
Suggested fix: For each permission beyond what the fullsend role already grants, document the specific e2e test operation that requires it (ideally in an ADR or inline comment). In particular: (1) Justify organization_administration: write — if tests only need repo-level admin, remove it. (2) Justify secrets: write — document whether tests must create/rotate secrets. (3) Justify members: write — if tests do not manage org membership, downscope to read. (4) Consider adding a code-level constraint restricting the e2e role to known test-pool orgs.
| // ValidRoles returns the set of recognized agent roles. | ||
| func ValidRoles() []string { | ||
| return []string{"fullsend", "triage", "coder", "review", "fix", "retro", "prioritize"} | ||
| return []string{"fullsend", "triage", "coder", "review", "fix", "retro", "prioritize", "e2e"} |
There was a problem hiding this comment.
[medium] scope-ambiguity
Adding e2e to ValidRoles() makes it configurable in any org's defaults.roles or per-repo roles list via config YAML. The mint handler has ALLOWED_ROLES and ROLE_APP_IDS environment-variable controls per mint instance, but there is no code-level guard at the config validation layer restricting which orgs may reference the e2e role. Issue #2155 explicitly constrains: 'Limit minted e2e credentials to the dedicated test org pool.'
Suggested fix: Ensure the e2e role's GitHub App and PEM are deployed only to mint instances serving the dedicated test org pool, or add explicit org-allowlist checking for the e2e role.
|
🤖 Finished Review · ✅ Success · Started 6:30 AM UTC · Completed 6:46 AM UTC |
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Assert canonical e2e permissions, ValidRoles/mintcore alignment, and AgentAppConfig parity with mintcore. Tracks consolidation in fullsend-ai#2449. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
e2e is a pool/CI mint role, not an installed agent app. Treat it like fullsend so harness generation does not look for harness/e2e.yaml. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
| "retro": {"actions": "read", "contents": "read", "pull_requests": "write", "issues": "write", "metadata": "read"}, | ||
| "prioritize": {"contents": "read", "issues": "write", "organization_projects": "write", "metadata": "read"}, | ||
| "fullsend": {"actions": "write", "actions_variables": "read", "contents": "write", "pull_requests": "write", "workflows": "write", "metadata": "read"}, | ||
| "e2e": { |
There was a problem hiding this comment.
[high] permission-expansion
The new e2e role grants administration: write, organization_administration: write, secrets: write, and members: write — the most privileged GitHub App permissions available. No other role in the canonical permissions map requests organization_administration or secrets at write level. Combined, a minted e2e token could reconfigure repos, manage org settings, overwrite secrets, and modify org membership. The mint handler's ALLOWED_ROLES/ROLE_APP_IDS gating and exclusion from DefaultAgentRoles() are meaningful mitigating controls, but neither the code nor an ADR documents why each elevated permission is necessary, and no code-level constraint restricts the role to known test-pool orgs.
Suggested fix: For each permission beyond what the fullsend role already grants, document the specific e2e test operation that requires it (ideally in an ADR or inline comment). In particular: (1) Justify organization_administration: write — if tests only need repo-level admin, remove it. (2) Justify secrets: write — document whether tests must create/rotate secrets. (3) Justify members: write — if tests do not manage org membership, downscope to read. (4) Consider adding a code-level constraint restricting the e2e role to known test-pool orgs.
| // ValidRoles returns the set of recognized agent roles. | ||
| func ValidRoles() []string { | ||
| return []string{"fullsend", "triage", "coder", "review", "fix", "retro", "prioritize"} | ||
| return []string{"fullsend", "triage", "coder", "review", "fix", "retro", "prioritize", "e2e"} |
There was a problem hiding this comment.
[medium] scope-inconsistency
The e2e role is added to ValidRoles() but intentionally excluded from DefaultAgentRoles() and PerRepoDefaultRoles(). ValidRoles() is used for config validation, but e2e is a pool/CI mint role that should never appear in org config files. The comment in harnesswrappers.go states 'The e2e role is a pool/CI mint role and is not installed as an agent app', but ValidRoles() is not documented as including non-agent roles. See also: [scope-ambiguity] finding at this location.
Suggested fix: Add a comment to ValidRoles() clarifying it includes mint-only roles that are not installed as agent apps.
db2cd63 to
01c65cf
Compare
|
🤖 Finished Review · ✅ Success · Started 6:49 AM UTC · Completed 7:04 AM UTC |
| "retro": {"actions": "read", "contents": "read", "pull_requests": "write", "issues": "write", "metadata": "read"}, | ||
| "prioritize": {"contents": "read", "issues": "write", "organization_projects": "write", "metadata": "read"}, | ||
| "fullsend": {"actions": "write", "actions_variables": "read", "contents": "write", "pull_requests": "write", "workflows": "write", "metadata": "read"}, | ||
| "e2e": { |
There was a problem hiding this comment.
[high] permission-expansion
The new e2e role grants administration: write, organization_administration: write, secrets: write, and members: write — the most privileged GitHub App permissions available. No other role in the canonical permissions map requests organization_administration or secrets at write level. Combined, a minted e2e token could reconfigure repos, manage org settings, overwrite secrets, and modify org membership. The mint handler ALLOWED_ROLES/ROLE_APP_IDS gating and exclusion from DefaultAgentRoles() are meaningful mitigating controls, but neither the code nor an ADR documents why each elevated permission is necessary, and no code-level constraint restricts the role to known test-pool orgs.
Suggested fix: For each permission beyond what the fullsend role already grants, document the specific e2e test operation that requires it (ideally in an ADR or inline comment). In particular: (1) Justify organization_administration: write — if tests only need repo-level admin, remove it. (2) Justify secrets: write — document whether tests must create/rotate secrets. (3) Justify members: write — if tests do not manage org membership, downscope to read. (4) Consider adding a code-level constraint restricting the e2e role to known test-pool orgs.
| // ValidRoles returns the set of recognized agent roles. | ||
| func ValidRoles() []string { | ||
| return []string{"fullsend", "triage", "coder", "review", "fix", "retro", "prioritize"} | ||
| return []string{"fullsend", "triage", "coder", "review", "fix", "retro", "prioritize", "e2e"} |
There was a problem hiding this comment.
[low] scope-inconsistency
The e2e role is added to ValidRoles() but intentionally excluded from DefaultAgentRoles() and PerRepoDefaultRoles(). ValidRoles() is used for config validation, but e2e is a pool/CI mint role. ValidRoles() itself lacks a comment noting it includes mint-only roles. The new TestValidRoles_RecognizedByMintcore test provides drift detection.
Suggested fix: Add a comment to ValidRoles() clarifying it includes mint-only roles that are not installed as agent apps.
|
🤖 Finished Retro · ✅ Success · Started 7:11 AM UTC · Completed 7:17 AM UTC |
Retro: PR #2407 —
|
Bring in upstream e2e role (fullsend-ai#2407) and latest main. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Bring in upstream e2e role (fullsend-ai#2407) and latest main. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
e2etomintcorecanonical role permissions (org-admin-equivalent +actions_variables: readfor future cross-org policy reads)e2etoconfig.ValidRoles()so CLI validation and error messages stay alignedmintsrc/mintcore/github.go.embedPrerequisite for #2155: after merge + mint deploy, operators can run
fullsend mint add-role e2eto create/register the e2e app without pool org enrollment.Test plan
go test ./internal/mintcore/...go test ./internal/cli/... -run TestValidateMintSetupRolego test ./internal/dispatch/gcf/... -run EmbeddedMintSourceFollow-up
After this merges and mint is redeployed:
fullsend mint add-role e2e --org …(or--slug+--pem)mainadmin foreign allowMade with Cursor