feat(mint): add --pem-dir to bootstrap PEMs during deploy by waynesun09 · Pull Request #1690 · fullsend-ai/fullsend

waynesun09 · 2026-05-29T14:14:16Z

Summary

Adds --pem-dir flag to mint deploy that reads {role}.pem files from disk, auto-discovers app IDs via unauthenticated GET /apps/{slug}, and stores PEMs in Secret Manager during deployment
After deploying with --pem-dir, mint enroll <org> works immediately without needing admin install first
Uses the default fullsend-ai app set; custom app sets continue to use admin install

Depends on

refactor!: rename --source-org flag to --app-set in mint enroll #1677 (rename --source-org to --app-set in enroll) — no code conflicts, but should merge first for consistent naming

Test plan

go vet ./internal/cli/... clean
go test ./internal/cli/ — all pass (including new TestLookupAppID_*, TestLoadAppSetPEMs_*, TestMintDeployCmd_PemDirFlag, TestMintDeployCmd_DryRunWithPemDir)
go build ./cmd/fullsend/ compiles
make lint clean
Integration: mint deploy --project=<PROJECT> --pem-dir=/path/to/pems --dry-run shows bootstrap plan
E2E: deploy fresh mint with PEMs, then mint enroll <org> succeeds without admin install

github-actions · 2026-05-29T14:15:55Z

Site preview

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

Commit: 2d86c83b40e02254779c0aa8a5870ff9731d1660

fullsend-ai-review · 2026-05-29T14:25:18Z

Review

Findings

High

[protected-path] .pre-commit-config.yaml — Protected infrastructure file modified without a linked issue. The change adds internal/cli/mint_test\.go$ to the detect-private-key exclusion list, which is mechanically justified by the fake PEM test fixtures in the new tests. However, human reviewers must verify protected-path changes regardless of justification, and the absence of a linked issue means there is no traceable authorization for this modification.
Remediation: Link a tracking issue that authorizes the .pre-commit-config.yaml change, or have a human reviewer explicitly approve this protected-path modification.

Low

[style] internal/cli/mint.go:54-55 — githubAPIBaseURL and githubHTTPClient are package-level mutable globals swapped via save/restore in tests. This follows existing patterns in this package but is fragile if tests ever run in parallel. An interface or function-parameter injection would be more robust. (Not urgent — the test file includes a comment documenting this constraint.)
Remediation: Consider passing an HTTP client and base URL via a struct or function parameters instead of package-level vars.

Previous run

Review

Findings

High

[protected-path] .pre-commit-config.yaml, skills/mint-enroll/SKILL.md — These are protected infrastructure/governance files that require human approval. The PR has no linked issue providing authorization context for these modifications. The .pre-commit-config.yaml change adds a test file exclusion for detect-private-key, and the skills update documents the new --pem-dir feature. Both changes are reasonable in context, but human reviewers must verify protected-path changes regardless.

Low

[correctness] internal/cli/mint.go:305-335 — The dry-run branch duplicates PEM existence and validity checks that loadAppSetPEMs already implements. Consider extracting the shared validation into a helper (e.g., validatePEMDir) to keep the dry-run path in sync with the real path. Not a bug today, but drift risk if roles or validation rules change.
Remediation: Extract the PEM directory validation (stat dir, check all role files exist, validate each PEM) into a shared function called by both the dry-run and real paths.
[style] internal/cli/mint.go:54-55 — githubAPIBaseURL and githubHTTPClient are package-level mutable globals swapped via save/restore in tests. This is a common Go pattern but fragile if tests ever run in parallel. An interface or function-parameter injection would be more robust.
Remediation: Consider passing an HTTP client and base URL via a struct or function parameters instead of package-level vars. Not urgent — follows existing patterns in this package.

Previous run (2)

Review

Findings

High

[protected-path] .pre-commit-config.yaml, skills/mint-enroll/SKILL.md — Protected governance/infrastructure files are modified without a linked issue. .pre-commit-config.yaml adds a detect-private-key exclusion for the new test file, and skills/mint-enroll/SKILL.md documents the PEM bootstrapping workflow. Both changes are coherent with the feature, but human approval is always required for protected-path changes and no issue provides authorization context.
Remediation: Link an issue that authorizes the protected-path changes, or obtain explicit human approval on these files.

Low

[correctness] internal/cli/mint.go:63 — lookupAppID uses http.DefaultClient which has no timeout. While the request context provides cancellation, a hung connection to the GitHub API could block indefinitely if no context deadline is set by the caller. Consider using a client with a reasonable timeout (e.g., 30s).
Remediation: Replace http.DefaultClient with a client that has a Timeout field set, or document that callers must set a context deadline.
[correctness] internal/cli/mint.go:94 — loadAppSetPEMs reads PEM files but does not validate that the file contents are actually PEM-encoded. A non-PEM file (e.g., a text file or binary) would be silently accepted and only fail later during cryptographic operations, producing a confusing error.
Remediation: Add a pem.Decode check after reading the file to verify at least one PEM block is present.

Previous run

Review

Findings

High

[protected-path] .pre-commit-config.yaml, skills/mint-enroll/SKILL.md — These are protected infrastructure/governance files that require human approval. The PR has no linked issue providing authorization context for these modifications. The .pre-commit-config.yaml change adds a test file exclusion for detect-private-key, and the skills update documents the new --pem-dir feature. Both changes are reasonable in context, but human reviewers must verify protected-path changes regardless.

Low

[correctness] internal/cli/mint.go:305-335 — The dry-run branch duplicates PEM existence and validity checks that loadAppSetPEMs already implements. Consider extracting the shared validation into a helper (e.g., validatePEMDir) to keep the dry-run path in sync with the real path. Not a bug today, but drift risk if roles or validation rules change.
Remediation: Extract the PEM directory validation (stat dir, check all role files exist, validate each PEM) into a shared function called by both the dry-run and real paths.
[style] internal/cli/mint.go:54-55 — githubAPIBaseURL and githubHTTPClient are package-level mutable globals swapped via save/restore in tests. This is a common Go pattern but fragile if tests ever run in parallel. An interface or function-parameter injection would be more robust.
Remediation: Consider passing an HTTP client and base URL via a struct or function parameters instead of package-level vars. Not urgent — follows existing patterns in this package.

Previous run (2)

Review

Findings

High

[protected-path] .pre-commit-config.yaml, skills/mint-enroll/SKILL.md — Protected governance/infrastructure files are modified without a linked issue. .pre-commit-config.yaml adds a detect-private-key exclusion for the new test file, and skills/mint-enroll/SKILL.md documents the PEM bootstrapping workflow. Both changes are coherent with the feature, but human approval is always required for protected-path changes and no issue provides authorization context.
Remediation: Link an issue that authorizes the protected-path changes, or obtain explicit human approval on these files.

Low

[correctness] internal/cli/mint.go:63 — lookupAppID uses http.DefaultClient which has no timeout. While the request context provides cancellation, a hung connection to the GitHub API could block indefinitely if no context deadline is set by the caller. Consider using a client with a reasonable timeout (e.g., 30s).
Remediation: Replace http.DefaultClient with a client that has a Timeout field set, or document that callers must set a context deadline.
[correctness] internal/cli/mint.go:94 — loadAppSetPEMs reads PEM files but does not validate that the file contents are actually PEM-encoded. A non-PEM file (e.g., a text file or binary) would be silently accepted and only fail later during cryptographic operations, producing a confusing error.
Remediation: Add a pem.Decode check after reading the file to verify at least one PEM block is present.

Previous run (3)

Review

Findings

High

[protected-path] .pre-commit-config.yaml — This PR modifies a protected infrastructure file (.pre-commit-config.yaml) and has no linked issue. The change adds internal/cli/mint_test\.go$ to the detect-private-key exclusion list, which is mechanically justified by the fake PEM test fixtures in the new tests. However, protected-path changes require human approval regardless of justification, and the absence of a linked issue means there is no traceable authorization for this modification.
Remediation: Link a tracking issue that authorizes the .pre-commit-config.yaml change, or have a human reviewer explicitly approve this protected-path modification.

Low

[correctness] internal/cli/mint.go — The --dry-run path with --pem-dir does not validate that the PEM directory exists or contains the expected files. A user running mint deploy --project=X --dry-run --pem-dir=/nonexistent gets a success message ("Would bootstrap app set...") with no indication the path is invalid. The non-dry-run path correctly fails on os.ReadFile, so this is cosmetic — but dry-run is meant to preview what would happen, and silently accepting an invalid path undermines that.
Remediation: Add a lightweight validation (e.g., os.Stat(pemDir)) in the dry-run branch before printing the preview message.

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review · 2026-05-29T15:33:46Z

+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		return 0, fmt.Errorf("looking up app %s: %w", slug, err)
+	}


[low] correctness

lookupAppID uses http.DefaultClient which has no timeout. A hung connection to the GitHub API could block indefinitely if no context deadline is set by the caller.

Suggested fix: Replace http.DefaultClient with a client that has a Timeout field set (e.g., 30s), or document that callers must set a context deadline.

fullsend-ai-review · 2026-05-29T15:33:47Z

+
+	roles := defaultMintRoles()
+	agentPEMs := make(map[string][]byte, len(roles))
+	agentAppIDs := make(map[string]string, len(roles))


[low] correctness

loadAppSetPEMs reads PEM files but does not validate that the file contents are actually PEM-encoded. A non-PEM file would be silently accepted and only fail later during cryptographic operations.

Suggested fix: Add a pem.Decode check after reading the file to verify at least one PEM block is present.

Fresh mint deploy creates infrastructure but no PEM secrets, so mint enroll fails until admin install runs. Adding --pem-dir lets operators provide PEM files at deploy time — the CLI auto-discovers app IDs from the GitHub API and stores everything in Secret Manager. After deploying with PEMs, mint enroll works immediately without needing admin install first. Signed-off-by: Wayne Sun <gsun@redhat.com>

The test file uses fake PEM keys for testing the --pem-dir flag. Signed-off-by: Wayne Sun <gsun@redhat.com>

Add deploy-time safeguards for --pem-dir: - Validate PEM format via appsetup.ValidateRSAPEM before storing - Verify each PEM matches its GitHub App via JWT-authenticated GET /app - Add directory existence check and pre-flight file existence check - Show expected vs found PEM filenames in error messages - Handle GitHub API rate limits (403/429) with clear guidance - Use dedicated HTTP client with 30s timeout instead of DefaultClient - Drain response bodies to enable connection reuse - Clarify --pem-dir is optional (first-time bootstrap only) in docs Signed-off-by: Wayne Sun <gsun@redhat.com>

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review · 2026-05-29T15:41:03Z

@@ -102,6 +305,36 @@ func newMintDeployCmd() *cobra.Command {
 				if skipDeploy {


[low] correctness

The dry-run branch duplicates PEM directory validation logic (stat dir, check role files exist, validate each PEM) that loadAppSetPEMs already implements. Drift risk if validation rules change.

Suggested fix: Extract shared PEM directory validation into a helper function called by both the dry-run and real code paths.

fullsend-ai-review · 2026-05-29T15:41:03Z

@@ -41,6 +52,193 @@ func resolveRole(role string) string {
 	return role
 }



[low] style

githubAPIBaseURL and githubHTTPClient are package-level mutable globals swapped via save/restore in tests. Fragile if tests ever run in parallel.

Suggested fix: Consider passing HTTP client and base URL via a struct or function parameters instead of package-level vars.

…tion - Extract ~30 lines of filesystem validation (dir exists, is dir, role PEM files exist, PEM format valid) into validatePEMDir, shared by both dry-run and loadAppSetPEMs paths - Add defense-in-depth: verifyPEMMatchesApp now decodes the GET /app response body and verifies the returned app ID matches expectations - Add dry-run message noting API verification is skipped - Add comment about GitHubOrgs/app-set name coupling - Add test parallelism guard comment Signed-off-by: Wayne Sun <waynesun09@gmail.com> Signed-off-by: Wayne Sun <gsun@redhat.com>

Covers the new defense-in-depth branch where GET /app returns 200 but with a different app ID than expected. Signed-off-by: Wayne Sun <waynesun09@gmail.com> Signed-off-by: Wayne Sun <gsun@redhat.com>

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review · 2026-05-29T15:52:50Z

      - id: trailing-whitespace
      - id: detect-private-key
-        exclude: "internal/layers/secrets_test\\.go$|internal/security/scanner_test\\.go$|internal/dispatch/gcf/provisioner_test\\.go$|tests/.*test_.*\\.py$"
+        exclude: "internal/layers/secrets_test\\.go$|internal/security/scanner_test\\.go$|internal/dispatch/gcf/provisioner_test\\.go$|internal/cli/mint_test\\.go$|tests/.*test_.*\\.py$"


[high] protected-path

Protected infrastructure file modified without a linked issue. The change adds internal/cli/mint_test.go$ to the detect-private-key exclusion list, justified by fake PEM test fixtures. Human reviewers must verify protected-path changes regardless, and the absence of a linked issue means there is no traceable authorization.

Suggested fix: Link a tracking issue that authorizes the .pre-commit-config.yaml change, or have a human reviewer explicitly approve this protected-path modification.

fullsend-ai-review · 2026-05-29T15:52:50Z

@@ -41,6 +52,216 @@ func resolveRole(role string) string {
 	return role
 }



[low] style

githubAPIBaseURL and githubHTTPClient are package-level mutable globals swapped via save/restore in tests. Fragile if tests ever run in parallel.

Suggested fix: Consider passing an HTTP client and base URL via a struct or function parameters instead of package-level vars.

github-actions Bot deployed to site-preview May 29, 2026 14:15 View deployment

github-actions Bot deployed to site-preview May 29, 2026 14:21 View deployment

fullsend-ai-review Bot suggested changes May 29, 2026

View reviewed changes

Comment thread .pre-commit-config.yaml

waynesun09 marked this pull request as ready for review May 29, 2026 15:28

fullsend-ai-review Bot suggested changes May 29, 2026

View reviewed changes

waynesun09 force-pushed the mint-deploy-pem-bootstrap branch from f8d4050 to 76f1d98 Compare May 29, 2026 15:34

waynesun09 added 3 commits May 29, 2026 11:34

fix: exclude mint_test.go from detect-private-key hook

fee4c96

The test file uses fake PEM keys for testing the --pem-dir flag. Signed-off-by: Wayne Sun <gsun@redhat.com>

waynesun09 force-pushed the mint-deploy-pem-bootstrap branch from 76f1d98 to e7011d0 Compare May 29, 2026 15:34

github-actions Bot deployed to site-preview May 29, 2026 15:36 View deployment

fullsend-ai-review Bot suggested changes May 29, 2026

View reviewed changes

github-actions Bot deployed to site-preview May 29, 2026 15:45 View deployment

test(mint): add app ID mismatch test for verifyPEMMatchesApp

2d86c83

Covers the new defense-in-depth branch where GET /app returns 200 but with a different app ID than expected. Signed-off-by: Wayne Sun <waynesun09@gmail.com> Signed-off-by: Wayne Sun <gsun@redhat.com>

github-actions Bot deployed to site-preview May 29, 2026 15:48 View deployment

waynesun09 added this pull request to the merge queue May 29, 2026

fullsend-ai-review Bot suggested changes May 29, 2026

View reviewed changes

Merged via the queue into main with commit c9f4660 May 29, 2026
9 of 10 checks passed

waynesun09 deleted the mint-deploy-pem-bootstrap branch May 29, 2026 15:56

ralphbean mentioned this pull request May 29, 2026

e2e: add mint installation and usage test coverage #1707

Open

This was referenced Jun 30, 2026

feat(cli): add fullsend agent subcommand (ADR 0058 Phase 2) #2770

Merged

feat(repos): add manifest parser and validation for repos.yaml #3002

Merged

fullsend-ai-retro Bot mentioned this pull request Jul 7, 2026

Evidence for #2401: 4 retro runs on release-service PR #1690, each reaching the same no-proposals conclusion #3278

Closed

guyoron1 mentioned this pull request Jul 7, 2026

Evidence for #2401: 4 retro runs on release-service PR #1690, each reaching the same no-proposals conclusion guyoron1/fullsend#107

Closed

github-actions Bot mentioned this pull request Jul 7, 2026

Evidence for #2993: protected-path severity oscillated between [high] and below-threshold across 12 reviews on release-service PR #1690 guyoron1/fullsend#108

Closed

		@@ -102,6 +305,36 @@ func newMintDeployCmd() *cobra.Command {
		if skipDeploy {

		@@ -41,6 +52,193 @@ func resolveRole(role string) string {
		return role
		}

		@@ -41,6 +52,216 @@ func resolveRole(role string) string {
		return role
		}

Uh oh!

Conversation

waynesun09 commented May 29, 2026

Summary

Depends on

Test plan

Uh oh!

github-actions Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

fullsend-ai-review Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

High

Low

Review

Findings

High

Low

Review

Findings

High

Low

Review

Findings

High

Low

Review

Findings

High

Low

Review

Findings

High

Low

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions Bot commented May 29, 2026 •

edited

Loading

fullsend-ai-review Bot commented May 29, 2026 •

edited

Loading