feat: add per-repo installation mode (ADR 0033)#799
Conversation
|
fullsend review is working on this — view logs |
Site previewPreview: https://c5a259cf-site.fullsend-ai.workers.dev Commit: |
Review: #799Head SHA: 07a7d2a SummaryThis PR adds per-repo installation mode (ADR 0033) with substantial changes across CLI, GCP provisioning, workflow dispatch, and scaffold templates. The implementation is well-structured with strong security practices: all user-controlled workflow inputs flow through FindingsMedium
Low
Info
FooterOutcome: comment-only Previous runReview: #799Head SHA: 255699f SummaryThis PR adds per-repo installation mode across the full stack: CLI ( FindingsMediumNone Low
Info
FooterOutcome: approve
Previous run (2)Review: #799Head SHA: b213c6b SummaryThis PR adds per-repo installation mode — a significant and well-structured feature spanning workflow routing, CLI commands, WIF provisioning, and config management. The Go code is well-tested (12 unit tests for config, comprehensive provisioner tests, CLI flag validation tests) and the security-sensitive workflow code properly uses FindingsHigh
Medium
Low
Info
FooterOutcome: request-changes Previous run (3)Review: #799Head SHA: 11e9436 SummaryThis PR adds per-repo installation mode (ADR 0033) with a well-structured design: a new FindingsHigh
MediumNone. Low
Info
FooterOutcome: request-changes Previous run (4)Review: #799Head SHA: 4cae81e SummaryThis PR adds a well-structured per-repo installation mode with solid input validation, comprehensive test coverage (12+ new tests), a good yq injection fix, and clean refactoring of token scope checks. However, all workflow FindingsCritical
Medium
Info
FooterOutcome: request-changes Previous run (5)Review: #799Head SHA: fb2aa05 SummaryThis is a large, well-structured feature PR that adds per-repo installation mode alongside the existing per-org mode. The Go code is solid — good input validation, comprehensive tests (12+ new unit tests for config, 15+ for provisioner/WIF, 10+ for CLI), and clean separation of concerns. The workflow security posture is strong: all user-controlled inputs flow through FindingsHigh
Medium
Low
Info
FooterOutcome: request-changes Previous run (6)Review: #799Head SHA: 0fcbbdc SummaryThis is a substantial, well-structured PR that adds per-repo installation mode alongside the existing per-org mode. The Go code is solid with thorough validation, good test coverage (12+ unit tests for config, extensive provisioner tests, CLI flag tests), and clean separation of concerns. The WIF auto-provisioning logic is idempotent and handles re-install gracefully. However, all reusable workflow and action references are hardcoded to the feature branch ( FindingsCritical
High
Medium
Low
Info
FooterOutcome: request-changes Previous run (7)Review: #799Head SHA: a068690 SummaryThis PR adds per-repo installation mode alongside the existing per-org mode — a substantial and well-structured feature with good test coverage (12 config tests, comprehensive provisioner tests, CLI validation tests). The Go code is solid: input validation is thorough, WIF auto-provisioning is idempotent, the yq command injection fix is a genuine security improvement, and secrets are zeroed after use. However, all 31 reusable workflow FindingsCritical
Medium
Low
Info
FooterOutcome: request-changes Previous run (8)Review: #799Head SHA: 5e8b76c SummaryThis is a well-structured, large feature PR adding per-repo installation mode with solid test coverage and good security practices in the workflow design (input sanitization via FindingsCritical
High(none) Medium
Low
Info
FooterOutcome: request-changes Previous run (9)Review: #799Head SHA: 4e7e074 SummaryThis PR adds a comprehensive per-repo installation mode alongside the existing per-org mode, including a new reusable dispatch workflow, CLI command routing, WIF auto-provisioning, shared app PEM handling, and a PerRepoConfig struct with thorough unit tests. The code quality is generally high — input validation is strong, user-controlled inputs in workflows use FindingsCritical
Medium
Low
Info
FooterOutcome: request-changes Previous run (10)Review: #799Head SHA: 31ed3d6 SummaryThis PR adds a per-repo installation mode (ADR 0033) spanning CLI commands, WIF auto-provisioning, config structures, workflow dispatch, and scaffold templates. The Go code is well-structured with thorough input validation, proper secret zeroing, and comprehensive test coverage (12 PerRepoConfig tests, extensive provisioner tests, CLI flag validation tests). However, every reusable workflow and action reference has been changed from FindingsHigh
Medium
Info
FooterOutcome: request-changes Previous run (11)Review: #799Head SHA: fde4033 SummaryThis PR adds per-repo installation mode (ADR 0033) with substantial Go CLI changes, WIF auto-provisioning, per-repo config, scaffold templates, and workflow routing. The Go code is well-structured with thorough input validation, good test coverage (12+ new unit tests for config, 15+ for provisioner, comprehensive CLI flag tests), and proper security practices (env-var-based input passing in workflows, yq injection fix, fork PR detection). However, all workflow refs and action pins have been changed from stable tags to the feature branch FindingsCritical
High
Medium
Low
Info
PR-specific checks
FooterOutcome: request-changes |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
waynesun09
force-pushed
the
feat-per-repo-installation
branch
from
7748e70 to
d26b9b5
Compare
|
fullsend review is working on this — view logs |
Per-repo mode e2e test results (nonflux/integration-service)Tested the full per-repo installation and dispatch chain against Setup
CLI tests
End-to-end dispatch chain
All agent run failures are the openshell gateway incompatibility (#780), not related to per-repo mode. Issues found and fixed
Workflow runs
|
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
|
fullsend review is working on this — view logs |
waynesun09
force-pushed
the
feat-per-repo-installation
branch
from
fb2aa05 to
4cae81e
Compare
|
fullsend review is working on this — view logs |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/u/271249281?s=60&v=4){kind=small}
waynesun09
force-pushed
the
feat-per-repo-installation
branch
from
4cae81e to
11e9436
Compare
|
fullsend review is working on this — view logs |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Review Summary
This PR successfully implements ADR 0033 (per-repo installation mode) with comprehensive test coverage and proper security considerations. The implementation is well-structured with clean separation between per-org and per-repo modes.
Strategic Assessment: Good idea, excellent scope fit, directly implements #727 and accepted ADR 0033.
One actionable request: The scaffold copy of the fullsend action has diverged from the live copy (missing OPENSHELL_VERSION fix).
Seven deferred notes for follow-up (not blocking):
CopyAgentPEMdoes not zero PEM key data after reading from Secret ManagercopySharedAppPEMssilently swallows API errors fromGetExistingRoleAppIDsandListOrgInstallationsundeleteWIFProvidererror silently swallowed with redundant duplicateUpdateWIFProvidercall — simplify to_ = c.undeleteWIFProvider(...); return c.UpdateWIFProvider(...)- Concurrency key in shim template may be empty for
pull_request_reviewevents, serializing unrelated PR reviews - Workspace prep logic (~20 lines) is copy-pasted across 5 reusable workflows — consider extracting into a composite action
ProvisionWIFinjectsRepofield into CEL expression without validation (CLI validates upstream, but the method is public)- Note: PR #742 (
/fs-prefix rename) is still open — when it merges, this dispatch workflow will need updating
Already flagged by prior review (ralphbean): Branch refs @feat-per-repo-installation must be changed to stable tag before merge. Not re-raised here.
| pemData, err := p.gcpAPI.AccessSecretVersion(ctx, p.cfg.ProjectID, srcID) | ||
| if err != nil { | ||
| return fmt.Errorf("reading source secret %s: %w", srcID, err) | ||
| } |
There was a problem hiding this comment.
[critical] (deferred) pemData read from AccessSecretVersion is never zeroed after use. The existing Provision method has zeroPEMs() for exactly this purpose. Consider:
defer func() {
for i := range pemData {
pemData[i] = 0
}
}()| } | ||
|
|
||
| roleSet := make(map[string]bool, len(roles)) | ||
| for _, r := range roles { |
There was a problem hiding this comment.
[important] (deferred) Both GetExistingRoleAppIDs and ListOrgInstallations errors are silently swallowed here (returning nil, nil). If these fail due to permissions or transient issues, the install proceeds without copying shared PEMs, which could cause a confusing failure later when the PEM secret is not found during app setup. Consider at minimum logging a warning via printer.StepWarn().
|
|
||
| if resp.StatusCode == http.StatusConflict { | ||
| io.Copy(io.Discard, io.LimitReader(resp.Body, 1<<20)) | ||
| if err := c.undeleteWIFProvider(ctx, projectNumber, poolID, providerID); err == nil { |
There was a problem hiding this comment.
[moderate] (deferred) Both branches call UpdateWIFProvider, and the undelete error is discarded. Consider simplifying to:
_ = c.undeleteWIFProvider(ctx, projectNumber, poolID, providerID)
return c.UpdateWIFProvider(ctx, projectNumber, poolID, providerID, cfg)| group: fullsend-dispatch-${{ github.event.issue.number || github.event.pull_request.number }} | ||
| cancel-in-progress: false | ||
| if: >- | ||
| github.event_name != 'issue_comment' |
There was a problem hiding this comment.
[moderate] (deferred) For pull_request_review events, github.event.issue is not populated and github.event.pull_request.number needs to be the fallback. If both are empty, all review-triggered runs share the same concurrency group. Consider adding github.event.number as an additional fallback:
group: fullsend-dispatch-${{ github.event.issue.number || github.event.pull_request.number || github.event.number }}| - name: Prepare workspace (upstream defaults + org overrides) | ||
| - name: Prepare workspace (upstream defaults + org/repo overrides) | ||
| env: | ||
| INSTALL_MODE: ${{ inputs.install_mode }} |
There was a problem hiding this comment.
[moderate] (deferred) The install_mode validation + CUSTOM_BASE path switching block (~20 lines) is duplicated identically across all 5 reusable workflow files. Consider extracting into a composite action (e.g., .github/actions/prepare-workspace/) to reduce maintenance burden and divergence risk.
| } | ||
| seen[lower] = true | ||
| orgs[i] = lower | ||
| } |
There was a problem hiding this comment.
[moderate] (deferred) The Repo field is injected directly into a CEL expression (assertion.repository == '%s'). While the CLI validates format upstream, ProvisionWIF is a public method — consider adding a format check here for defense in depth.
|
Re: scaffold if curl -LsSf .../install.sh | OPENSHELL_VERSION=v0.0.36 sh; thenThe remaining 6 comments are all marked (deferred) — no action needed on those this round. |
waynesun09
force-pushed
the
feat-per-repo-installation
branch
from
11e9436 to
b213c6b
Compare
|
fullsend review is working on this — view logs |
Add `fullsend admin install --repo owner/repo` for per-repo agent enrollment. Generates a shim workflow that routes events through reusable-dispatch.yml, configures repo variables/secrets, and validates enrollment via composite actions. Includes mode detection (per-org vs per-repo), cross-mode flag validation, agent role parsing, SA key zeroing, deterministic scaffold output, and per-repo config model with YAML round-trip. Signed-off-by: Wayne Sun <gsun@redhat.com>
When --gcp-auth-mode=wif is used without explicit --gcp-wif-provider, auto-provision a WIF pool, provider, and service account in the target GCP project. Supports multi-org binding with condition merging via GetWIFProvider + UpdateWIFProvider (PATCH-based). Build repoSecrets map after provisioning so WIF provider and SA email values are populated. Use printf over echo for untrusted COMMENT_BODY in reusable-fix.yml. Signed-off-by: Wayne Sun <gsun@redhat.com>
When a second org installs the same public GitHub Apps, the system now
automatically detects the shared apps by matching app IDs against the
existing mint function's ROLE_APP_IDS, and copies PEM secrets from the
source org's naming convention to the target org's. This eliminates the
need for --skip-app-setup or manual PEM provisioning when enrolling
additional orgs with shared apps.
Also adds per-repo WIF provider isolation: each per-repo install creates
its own WIF provider (gh-{owner}-{repo}) with a repo-scoped condition
instead of reusing the shared github-oidc provider.
Signed-off-by: Wayne Sun <gsun@redhat.com>
The releases API call to resolve the latest tag was unauthenticated, hitting the 60 req/hour rate limit on shared runner IPs. Add a github_token input (defaults to github.token) and pass it as a Bearer token on the API call. Signed-off-by: Wayne Sun <gsun@redhat.com>
Adapt per-repo installation to the direct-WIF model: remove SA key auth path (WIF is the only GCP auth mode), remove stale SA email secret and dead gcp-service-account flag, use project-level IAM bindings with roles/aiplatform.user, and scope per-org WIF bindings to org/.fullsend instead of org-wide. Ref: #884 Signed-off-by: Wayne Sun <gsun@redhat.com> NEWMSG "$@" Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
feat-per-repo-installation
branch
from
b213c6b to
5e36b45
Compare
|
fullsend review is working on this — view logs |
Add provisioner_test.go to detect-private-key exclusion and quote
CUSTOM_BASE expansion inside ${f#...} across all reusable workflows.
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
The per-org WorkflowsLayer.Install() never created customized/ subdirectories, so orgs installed before env/ was added to layeredDirs are missing customized/env/. Add CustomizedDirs() helper and use it in both the install and analyze paths so reinstalls are self-healing. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
fullsend review is working on this — view logs |
2 participants
Summary
reusable-dispatch.ymlfor centralized per-repo event routing — combines routing logic with conditionalworkflow_calljobs, including kill switch check, role filtering, and fork PR detectioninstall_modeinput (per-orgdefault;per-reposwitchescustomized/overlay path to.fullsend/customized/)install_modevalidation to workspace prep steps andvalidate-enrollmentaction; fix yq command injection usingenv()bindingPerRepoConfigstruct with parse/validate/marshal, per-repo shim template,fullsend admin install --repoCLI command, and scaffold helpersPerRepoConfigcovering parsing, validation, marshaling, and round-trip--gcp-auth-mode=wifis used without explicit--gcp-wif-provider: creates WIF pool, OIDC provider, and service account, then binds the principal for the target orgGetWIFProvider, merges org sets, and PATCHes the provider viaUpdateWIFProviderfullsend-mintsogoogle-github-actions/authWIF mode works out of the box; on re-install, PATCH existing providers with stale audiencesrepoSecretsmap afterProvisionWIFso WIF provider and SA email values are populated before writing to GitHubPer-repo flow
3 levels of
workflow_callnesting (within GitHub's 4-level limit). No config repo needed —.fullsend/config.yamland.fullsend/customized/live in the target repo.WIF auto-provisioning
When
--gcp-auth-mode=wifis specified without--gcp-wif-provider/--gcp-wif-sa-email, the install command auto-provisions:fullsend-dispatch@{project}.iam.gserviceaccount.comfullsend-poolwith GitHub OIDC issuergithub-oidcwithassertion.repository_owner == '{org}'condition and dual audiences (fullsend-mint+ IAM)attribute.repository_ownerprincipalOn re-install, existing resources are detected (409 → fetch + merge + PATCH) — idempotent and safe for multi-org projects.
Test plan
go test ./...) — config 96.2%, gcf 75.9%, scaffold 73.3%go vetcleanfullsend admin install --repo --dry-runagainstnonflux/integration-service— prints correct shim, config, vars, and secretsfullsend admin install --repowrites shim +.fullsend/config.yaml+customized/gitkeeps to target repoFULLSEND_MINT_URL,FULLSEND_GCP_REGION,FULLSEND_GCP_AUTH_MODE) and secrets (FULLSEND_GCP_WIF_PROVIDER,FULLSEND_GCP_WIF_SA_EMAIL,FULLSEND_GCP_PROJECT_ID) set on target repo/triage— full chain passes: shim → reusable-dispatch → reusable-triage → workspace layering → enrollment skip → mint → GCP auth → agent setup (fails at agent run, Investigate standalone openshell-gateway with Docker/Podman drivers across target environments #780)/code— routes to code stage, mint issues coder token, all steps pass through agent setup (Investigate standalone openshell-gateway with Docker/Podman drivers across target environments #780)/review— routes to review stage, mint issues review token, all steps pass (Investigate standalone openshell-gateway with Docker/Podman drivers across target environments #780)/fix— routes to fix stage, fork PR check passes, no-fix label check passes, all steps pass (Investigate standalone openshell-gateway with Docker/Podman drivers across target environments #780)/stop-fix— runs in shim (not dispatched),fullsend-no-fixlabel applied, workflow succeedsinstall_mode=per-repocorrectly skips enrollment check (logs: "Per-repo mode — skipping config.yaml enrollment check").fullsend/customized/path in per-repo mode.fullsend/config.yamlin target repo checkout.fullsend/config.yamlworkflow_callnesting works (shim → reusable-dispatch → reusable-stage)repoSecretsmap populated after provisioning (not before)Issues found during testing
WIF provider audience mismatch — the WIF provider was created with only
fullsend-mintas an allowed audience.google-github-actions/auth@v3uses the default IAM audience (https://iam.googleapis.com/projects/{number}/...), which didn't match. Fixed:AllowedAudiencesnow includes bothfullsend-mint(for token mint) and the IAM audience (forgoogle-github-actions/auth). On re-install, existing providers with stale audiences are automatically PATCHed.Repo secrets written as empty strings —
repoSecretsmap was built beforeProvisionWIFran, so WIF provider and SA email were empty. Fixed: map construction moved to after provisioning.Test environment