refactor(repos): extract per-repo install logic into internal/repos package

[ggallen]

Summary

• Extract the core per-repo installation flow (guard check, WIF provisioning, scaffold commit, variable/secret writes) from internal/cli/admin.go into a new internal/repos/ package
• Introduce repos.Install() with InstallConfig struct and WIFProvisioner interface, decoupling install logic from CLI concerns (spinners, prompts, flag parsing) and from the concrete GCF provisioner
• Modify admin.go to delegate to repos.Install() via a gcfWIFAdapter bridge, preserving existing behavior

This is PR 1 of the fullsend repos management feature (ADR 0057). It preserves install semantics -- existing fullsend github setup / fullsend admin install work exactly as before.

Test plan

• All 17 new tests pass (go test ./internal/repos/ -v)
• Test coverage is 89.2% (target: >85%)
• Full project builds cleanly (go build ./...)
• Existing CLI tests pass (go test ./internal/cli/)
• go vet clean on both packages
• Pre-commit hooks pass (gofmt, go vet, etc.)

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:11 AM UTC · Completed 12:25 AM UTC
Commit: c1b1e2c · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Extract per-repo install flow into reusable internal/repos.Install

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Extract per-repo install flow into new internal/repos package for reuse by future commands
• Introduce repos.Install with InstallConfig + WIFProvisioner to decouple CLI and GCF provisioning
• Update admin install path to delegate to repos.Install while preserving existing behavior

Diagram

graph TD
  A["internal/cli/admin.go"] --> B["internal/repos.Install"] --> C["forge.Client"] --> D{{"GitHub API"}}
  B --> E["repos.WIFProvisioner"] --> F["gcf.Provisioner"]
  B --> G["scaffold + config"]
  subgraph Legend
    direction LR
    _m["Module/Package"] ~~~ _e{{"External"}} ~~~ _i["Interface"]
  end

Loading

High-Level Assessment

The extraction into an internal/repos package with an InstallConfig + WIFProvisioner interface is the right direction for reuse (e.g., future bulk or non-interactive flows) while keeping CLI concerns out. Alternatives considered (an Installer struct with injected dependencies, or keeping helpers in internal/cli) would either complicate call sites or keep reuse tightly coupled to CLI semantics; the current functional API + adapter is the simplest reusable seam.

Files changed (3) +1044 / -26

Refactor (2) +462 / -26

admin.goDelegate per-repo install steps to repos.Install via GCF adapter +135/-26

Delegate per-repo install steps to repos.Install via GCF adapter

• Replaces inline WIF provisioning, scaffold commit, and repo var/secret writes with a call to repos.Install. Adds a progress callback to map install phases back into existing CLI step output, and introduces a gcfWIFAdapter to satisfy the new repos.WIFProvisioner interface. Preserves existing behavior, including vendor-specific extra commit handling.

internal/cli/admin.go

install.goAdd reusable per-repo installation API (Install/InstallConfig/WIFProvisioner) +327/-0

Add reusable per-repo installation API (Install/InstallConfig/WIFProvisioner)

• Introduces internal/repos with InstallConfig and Install() to run the per-repo install flow (optional guard check, optional mint discovery, optional WIF provisioning, scaffold generation + commit/PR delivery, and variable/secret writes). Exposes BuildScaffoldFiles for CLI dry-run paths and provides deterministic write ordering via sortedKeys.

internal/repos/install.go

Tests (1) +582 / -0

install_test.goComprehensive tests for install outcomes, error paths, and progress phases +582/-0

Comprehensive tests for install outcomes, error paths, and progress phases

• Adds a fake WIF provisioner and uses forge.FakeClient to validate direct vs PR delivery, guard-check behavior, mint discovery, WIF provisioning success/failure, scaffold commit failure, and progress callback phase ordering. Covers both success and partial-state scenarios to ensure refactor preserves semantics.

internal/repos/install_test.go

[github-actions]

Site preview

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

Commit: a2d225ffcacef72b2066b96b57e1759000541716

[codecov]

Codecov Report

❌ Patch coverage is 86.92308% with 34 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/admin.go	79.86%	25 Missing and 5 partials ⚠️
internal/repos/install.go	96.03%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. Vendored install non-atomic ✓ Resolved 🐞 Bug ≡ Correctness

Description

With --vendor, admin.go first runs repos.Install (which writes a vendored-mode shim
referencing local reusable workflows) and only then commits the vendored reusable workflows/binary
in a separate scaffold commit. If the second commit fails, the repo is left with workflows that
reference missing local reusable workflows, breaking automation until repaired.

Code

internal/cli/admin.go[R989-1071]

+	installCfg := repos.InstallConfig{
+		Owner:            owner,
+		Repo:             repo,
+		Roles:            roles,
+		MintURL:          mintURL,
+		InferenceProject: inferenceProject,
+		InferenceRegion:  inferenceRegion,
+		UpstreamRef:      upstreamRef,
+		UpstreamTag:      upstreamTag,
+		SkipMintCheck:    true, // already handled above
+		SkipAppSetup:     true, // already handled above
+		SkipGuardCheck:   true, // admin.go handles guard check itself (lines 691-704)
+		SkipWIF:          !needsWIFProvision,
+		WIFProvider:      inferenceWIFProvider,
+		VendorBinary:     vendor,
+		Direct:           c.Direct,
+	}
+
+	progressFn := func(_ string, phase, msg string) {
+		switch phase {
+		case "wif":
+			if strings.Contains(msg, "Provisioning") {
+				printer.StepStart(msg)
+			} else if strings.Contains(msg, "ready") {
+				printer.StepDone(msg)
+				printer.StepInfo("IAM policy changes may take up to 7 minutes to propagate")
+				printer.StepInfo("Agent workflows that authenticate via WIF may fail until propagation completes")
+			}
+		case "scaffold":
+			if strings.Contains(msg, "Committing") || strings.Contains(msg, "Generating") {
+				printer.StepStart(msg)
+			} else {
+				printer.StepDone(msg)
+			}
+		case "vars":
+			if strings.Contains(msg, "Configuring") {
+				printer.StepStart(msg)
+			} else {
+				printer.StepDone(msg)
+			}
+		case "secrets":
+			if strings.Contains(msg, "Configuring") {
+				printer.StepStart(msg)
+			} else {
+				printer.StepDone(msg)
+			}
		}
-		printer.StepDone("WIF infrastructure ready")
-		printer.StepInfo("IAM policy changes may take up to 7 minutes to propagate")
-		printer.StepInfo("Agent workflows that authenticate via WIF may fail until propagation completes")
	}

-	repoVars := map[string]string{
-		"FULLSEND_MINT_URL":   mintURL,
-		"FULLSEND_GCP_REGION": inferenceRegion,
-		forge.PerRepoGuardVar: "true",
+	installResult, installErr := repos.Install(ctx, installCfg, client, wifProv, progressFn)
+	if installErr != nil {
+		return installErr
	}

-	repoSecrets := map[string]string{
-		"FULLSEND_GCP_PROJECT_ID":   inferenceProject,
-		"FULLSEND_GCP_WIF_PROVIDER": inferenceWIFProvider,
+	if installResult.WIFProvider != "" {
+		inferenceWIFProvider = installResult.WIFProvider
	}

	if vendor {
		var vendorErr error
+		scaffoldFiles, buildErr := repos.BuildScaffoldFiles(installCfg)
+		if buildErr != nil {
+			return fmt.Errorf("building scaffold files for vendor: %w", buildErr)
+		}
+		files = scaffoldFiles
		files, _, vendorErr = appendVendorTreeFiles(printer, owner, repo, files, vendor, fullsendBinary, fullsendSource)
		if vendorErr != nil {
			return fmt.Errorf("collecting vendored assets: %w", vendorErr)
		}
-	}
-
-	if err := applyPerRepoScaffold(ctx, client, printer, owner, repo, files, repoVars, repoSecrets, c.Direct); err != nil {
-		return err
+		// Vendor files need a separate commit since they weren't included
+		// in the repos.Install scaffold commit.
+		repoVars := map[string]string{
+			"FULLSEND_MINT_URL":   mintURL,
+			"FULLSEND_GCP_REGION": inferenceRegion,
+			forge.PerRepoGuardVar: "true",
+		}
+		repoSecrets := map[string]string{
+			"FULLSEND_GCP_PROJECT_ID":   inferenceProject,
+			"FULLSEND_GCP_WIF_PROVIDER": inferenceWIFProvider,
+		}
+		if err := applyPerRepoScaffold(ctx, client, printer, owner, repo, files, repoVars, repoSecrets, c.Direct); err != nil {
+			return err
+		}

Relevance

⭐⭐⭐ High

Team previously accepted fixing non-atomic vendoring inconsistency windows in install flow.

PR-#1954

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

admin.go calls repos.Install before vendored files are appended/committed; vendored mode
rewrites reusable workflow references to local paths, which are only satisfied once the vendored
workflow files are committed.

internal/cli/admin.go[974-1072]
internal/repos/install.go[184-195]
internal/repos/install.go[297-299]
internal/scaffold/render.go[102-117]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The vendor path is split across two commits:
1) `repos.Install` commits scaffold files rendered in vendored mode.
2) `applyPerRepoScaffold` commits vendored workflows/assets.

Vendored-mode templates rewrite `uses:` to local workflow paths (e.g. `./.github/workflows/reusable-dispatch.yml`). After step (1) but before (2)—or permanently if (2) fails—the shim workflow points at local reusable workflows that do not exist yet.

### Issue Context
`repos.BuildScaffoldFiles` calls `scaffold.CollectPerRepoInstallFiles(cfg.VendorBinary, ...)`, which renders vendored mode when `VendorBinary=true`. Vendored mode switches reusable workflow references to local paths. Vendored workflow files are provided by `appendVendorTreeFiles` (vendoring) and must be present in the same commit/PR as the shim.

### Fix Focus Areas
- internal/cli/admin.go[989-1071]
- internal/repos/install.go[184-195]
- internal/repos/install.go[297-299]
- internal/scaffold/render.go[102-117]

### Recommended fix approach
Ensure the shim commit and vendored reusable workflow files are delivered atomically:
- Option A: Extend `repos.Install` to accept an optional `AdditionalScaffoldFiles` (or callback) that the CLI can use to provide the vendored file set, and commit them together in `commitScaffold`.
- Option B: For `vendor=true`, skip `repos.Install`’s scaffold commit entirely and instead:
 1) Build scaffold files in vendored mode.
 2) Append vendored files.
 3) Deliver everything in one `layers.CommitScaffoldFiles` call.
 4) Keep vars/secrets writes in one place (avoid double-writing).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Scaffold commit loses fallbacks ✓ Resolved 🐞 Bug ☼ Reliability

Description

repos.commitScaffold bypasses layers.CommitScaffoldFiles, so direct installs no longer retry
non-fast-forward pushes or fall back to a PR on protected branches, and PR installs no longer
support fork-based delivery for non-owner users. This can cause per-repo installs to fail in common
org setups (protected default branches or contributors without upstream push access).

Code

internal/repos/install.go[R229-281]

+// commitScaffold commits scaffold files using the appropriate strategy.
+func commitScaffold(ctx context.Context, client forge.Client, cfg InstallConfig,
+	files []forge.TreeFile, result *InstallResult, progress ProgressFunc) error {
+
+	repoFullName := cfg.Owner + "/" + cfg.Repo
+	targetRepo, err := client.GetRepo(ctx, cfg.Owner, cfg.Repo)
+	if err != nil {
+		return fmt.Errorf("getting repo info: %w", err)
+	}
+
+	commitMsg := "chore: initialize fullsend per-repo installation"
+	if cfg.Direct {
+		committed, commitErr := client.CommitFiles(ctx, cfg.Owner, cfg.Repo, commitMsg, files)
+		if commitErr != nil {
+			return fmt.Errorf("committing scaffold files: %w", commitErr)
+		}
+		if committed {
+			progress(repoFullName, "scaffold", fmt.Sprintf("Pushed scaffold files to %s", targetRepo.DefaultBranch))
+		} else {
+			progress(repoFullName, "scaffold", "Scaffold files up to date")
+		}
+		return nil
+	}
+
+	// PR-based delivery.
+	prTitle := "chore: initialize fullsend per-repo installation"
+	prBody := "This PR adds the fullsend scaffold files for per-repo installation.\n\n" +
+		"Merge this PR to activate fullsend workflows."
+	scaffoldBranch := "fullsend/scaffold-install"
+
+	if branchErr := client.CreateBranch(ctx, cfg.Owner, cfg.Repo, scaffoldBranch); branchErr != nil {
+		if !forge.IsAlreadyExists(branchErr) {
+			return fmt.Errorf("creating scaffold branch: %w", branchErr)
+		}
+	}
+
+	_, commitErr := client.CommitFilesToBranch(ctx, cfg.Owner, cfg.Repo, scaffoldBranch, commitMsg, files)
+	if commitErr != nil {
+		return fmt.Errorf("committing scaffold files to branch: %w", commitErr)
+	}
+
+	proposal, prErr := client.CreateChangeProposal(ctx, cfg.Owner, cfg.Repo,
+		prTitle, prBody, scaffoldBranch, targetRepo.DefaultBranch)
+	if prErr != nil {
+		if !forge.IsAlreadyExists(prErr) && !forge.IsNoChanges(prErr) {
+			return fmt.Errorf("creating scaffold PR: %w", prErr)
+		}
+		progress(repoFullName, "scaffold", "Scaffold PR up to date")
+	} else {
+		result.ScaffoldPR = proposal.URL
+		progress(repoFullName, "scaffold", fmt.Sprintf("Created scaffold PR: %s", proposal.URL))
+	}
+	return nil

Relevance

⭐⭐⭐ High

Repo has repeatedly accepted scaffold commit fallback/retry logic (protected branches, forks, non-FF
retries).

PR-#2201
PR-#2533
PR-#2594

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

repos.commitScaffold uses raw forge client calls and lacks any of the branch-protection fallback,
non-fast-forward retry, or fork-based PR handling that exists in the previously used scaffold
delivery implementation.

internal/repos/install.go[229-281]
internal/layers/commit.go[48-104]
internal/layers/commit.go[317-337]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`internal/repos.commitScaffold` reimplements scaffold delivery but drops key behaviors that `layers.CommitScaffoldFiles` already provides (non-fast-forward retry, branch-protection fallback to PR, and fork-based PR delivery for non-owner users). This creates real install failures when repos have protected default branches or when the installer lacks upstream push access.

### Issue Context
Historically, per-repo installs in `admin.go` went through `layers.CommitScaffoldFiles`, which explicitly handles:
- protected default branches by falling back to PR delivery
- auto-init races via non-fast-forward retry
- non-owner users via fork and cross-fork PR

### Fix Focus Areas
- internal/repos/install.go[229-281]
- internal/layers/commit.go[48-104]
- internal/layers/commit.go[317-337]

### Recommended fix approach
- Option A (preferred): Reuse `layers.CommitScaffoldFiles` from `repos` by introducing a small adapter layer that keeps `repos.Install` UI-agnostic (e.g., pass `nil`/no-op printer and `nil` input reader for non-interactive bulk installs), while preserving the delivery semantics.
- Option B: Port the missing fallback logic into `internal/repos`:
 - Direct mode: retry once on `forge.ErrNonFastForward`; if `forge.ErrBranchProtected`, switch to PR-based delivery.
 - PR mode: implement the same fork-vs-upstream decision path (default-to-fork for non-interactive callers), and use cross-fork PR head format when needed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Guard check fails open ✓ Resolved 🐞 Bug ☼ Reliability

Description

When GetRepoVariable fails during the guard check, repos.Install reports progress and continues
with installation even though comments say bulk installers use the guard to skip already-installed
repos. This makes bulk installs non-idempotent under transient/permission errors and can overwrite
or duplicate per-repo setup.

Code

internal/repos/install.go[R138-148]

+	// Step 1: Check guard variable to detect existing installations.
+	// When the guard is set to "true", the repo already has a per-repo
+	// installation. Bulk-install callers use this to skip already-installed
+	// repos. Interactive CLI callers set SkipGuardCheck=true because they
+	// handle the guard check themselves (and always proceed with updates).
+	if !cfg.SkipGuardCheck {
+		progress(repoFullName, "guard", "Checking installation status")
+		guardVal, guardExists, guardErr := client.GetRepoVariable(ctx, cfg.Owner, cfg.Repo, forge.PerRepoGuardVar)
+		if guardErr != nil {
+			progress(repoFullName, "guard", fmt.Sprintf("Could not check guard variable: %v", guardErr))
+		}

Relevance

⭐⭐⭐ High

Guard checks are expected not to fail open; prior PR accepted failing closed/handling
GetRepoVariable errors.

PR-#967

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The function’s own comments state the guard is used by bulk installers to skip already-installed
repos, but on guard lookup error it continues as if the guard were absent.

internal/repos/install.go[138-148]
PR-#967

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Install` treats guard-check errors as warnings and proceeds. But the guard check is documented as the mechanism bulk installers use to skip already-installed repos; proceeding on error defeats that safety mechanism.

### Issue Context
The current behavior is especially risky for future bulk install flows: a transient API error turns into a full install attempt (commits + secrets/vars writes) rather than a safe skip.

### Fix Focus Areas
- internal/repos/install.go[138-155]

### Recommended fix approach
- When `!cfg.SkipGuardCheck` and `GetRepoVariable` returns an error, return a non-nil error (fail closed) OR return `(result, err)` with a distinct error type so bulk callers can skip/report safely.
- If the interactive CLI truly wants to proceed despite guard errors, keep that behavior by setting `SkipGuardCheck=true` (as `admin.go` already does) or introduce an explicit `AllowGuardCheckFailure` flag.
- Update tests accordingly (notably the test that currently asserts install continues after guard error).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Hardcoded WIFProvider in tests ✓ Resolved 📘 Rule violation ⛨ Security

Description

internal/repos/install_test.go hardcodes GCP project/WIF provider identifiers (e.g.,
InferenceProject and WIFProvider) instead of using clearly fake placeholders. This risks
accidentally encoding environment-specific infrastructure identifiers in source control.

Code

internal/repos/install_test.go[R54-65]

+		Owner:            "acme",
+		Repo:             "widgets",
+		Roles:            []string{"triage", "coder"},
+		MintURL:          "https://mint.example.com",
+		InferenceProject: "acme-inference",
+		InferenceRegion:  "us-central1",
+		WIFProvider:      "projects/123/locations/global/workloadIdentityPools/pool/providers/prov",
+		Direct:           true,
+		SkipGuardCheck:   true,
+		SkipMintCheck:    true,
+		SkipWIF:          true,
+	}

Relevance

⭐⭐ Medium

No historical evidence found about rejecting real-looking cloud resource IDs in tests; security rule
exists but unclear enforcement.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance rule disallows hardcoded secrets and sensitive environment-specific identifiers in
source code, and requires test values to be clearly fake placeholders. The cited test code hardcodes
a GCP project-like ID (acme-inference) and WIF provider resource names (projects/123/...).

Rule 1062040: Disallow hardcoded secrets and sensitive environment-specific identifiers in source code
internal/repos/install_test.go[54-65]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests hardcode environment-specific identifiers (`InferenceProject`, `WIFProvider`) that could plausibly correspond to real GCP resources. Compliance requires avoiding hardcoded sensitive/environment-specific identifiers; test data should be clearly fake placeholders.

## Issue Context
This occurs in the newly added repos install tests.

## Fix Focus Areas
- internal/repos/install_test.go[54-65]
- internal/repos/install_test.go[317-320]
- internal/repos/install_test.go[413-415]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. WIF error drops result ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

On ProvisionWIF failure, repos.Install sets result.WIFProvider but returns (nil, err),
discarding the partially populated InstallResult. This is inconsistent with later error paths that
return (result, err) and prevents callers from inspecting partial state.

Code

internal/repos/install.go[R168-177]

+	// Step 3: WIF provisioning (unless SkipWIF or provider already set).
+	wifProvider := cfg.WIFProvider
+	if !cfg.SkipWIF && wifProvider == "" && provisioner != nil {
+		progress(repoFullName, "wif", "Provisioning WIF infrastructure")
+		var err error
+		wifProvider, err = provisioner.ProvisionWIF(ctx)
+		if err != nil {
+			result.WIFProvider = wifProvider // capture partial state
+			return nil, fmt.Errorf("provisioning WIF: %w", err)
+		}

Relevance

⭐⭐ Medium

No clear historical precedent on returning partial results vs nil on error in install-style APIs.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly assigns result.WIFProvider on error but then returns a nil result pointer,
which discards the captured value.

internal/repos/install.go[168-177]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Install` attempts to capture partial WIF provider state on provisioning failure, but returns a nil result pointer. This loses the captured state and makes error handling inconsistent with other failure paths.

### Issue Context
The function signature returns `(*InstallResult, error)`, and other failures (e.g. scaffold commit failures) return a non-nil result. WIF provisioning failure should follow the same convention.

### Fix Focus Areas
- internal/repos/install.go[168-177]

### Recommended fix approach
- Change the WIF error path to `return result, fmt.Errorf("provisioning WIF: %w", err)`.
- (Optional) Populate `result.Error = err` if that field is intended to be used.
- Update/extend tests to assert the non-nil result on WIF errors if desired.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Looks good to me

Findings

Low

• [api-shape-pattern] internal/repos/install.go:166 — Install() returns (*InstallResult, error) and consistently returns a non-nil result even on error paths for partial state inspection (e.g., WIFProvider). This is intentional and well-tested (e.g., TestInstall_WIFProvisioningFailure verifies non-nil result on error), but unconventional in Go where returning a non-nil result alongside a non-nil error can mislead callers.

• [documentation-drift] docs/plans/repos-management.md:367 — The plan document's InstallConfig struct still lists SkipMintDeploy (removed from implementation), MintProject, MintRegion (never added), and ScaffoldPR in InstallResult (not in implementation). The Install() signature also omits the commitScaffold callback parameter that exists in the real code. Since this is a living plan document referenced by ADR 0057, the divergence could cause confusion for future implementers.

• [test-coverage] internal/repos/install_test.go:239 — No test covers the case where SkipMintCheck=false and MintURL is non-empty (the "provided URL takes precedence over discovery" path). In Install(), the condition is !cfg.SkipMintCheck && mintURL == "", so when MintURL is already set, discovery is skipped even with SkipMintCheck=false. Indirectly tested by other tests but no dedicated coverage.

Previous run

Looks good to me

Findings

Low

• [logic-error] internal/repos/install.go:61 — The SkipMintDeploy field is declared in InstallConfig but never read by Install() or BuildScaffoldFiles(). Intended for a future PR per the repos-management plan, but an unused field can mislead callers into believing they can control mint deployment behavior.

• [testing-override-exposure] internal/cli/admin.go:163 — The testClient, testPrinter, and testWIFProvisioner fields on perRepoInstallConfig couple test infrastructure into the production struct. The code includes an explicit nil check (if c.testPrinter == nil { return fmt.Errorf(...) }) when testClient is set, mitigating the panic risk. All current tests use perRepoTestBase() which always initializes testPrinter.

• [logic-error] internal/cli/admin.go — The dry-run path constructs its own InstallConfig for BuildScaffoldFiles() that diverges from the non-dry-run installCfg. The code includes a comment documenting that BuildScaffoldFiles only reads Owner, Repo, Roles, VendorBinary, UpstreamRef, UpstreamTag. The divergence is harmless today.

• [scope-creep] docs/plans/repos-management.md — The PR updates the plan from "Zero behavioral change" to "Preserves install semantics". The PR body and plan document are now aligned on the weaker but more accurate claim. The testability additions mean this is not a pure zero-behavioral-change refactor, but it does preserve install semantics.

• [api-shape-pattern] internal/repos/install.go — Install() returns (*InstallResult, error) and consistently returns a non-nil result even on error for partial state inspection. This pattern is well-tested across multiple test cases. The ProgressFunc callback design intentionally decouples the repos package from the UI layer.

• [naming-consistency] internal/maputil/maputil.go — New package maputil contains a single helper function (SortedKeys). It is used across 4 files spanning 3 packages (cli, dispatch/gcf, repos), making a shared utility package the idiomatic Go approach.

Previous run (2)

Looks good to me

Findings

Medium

• [scope-creep] internal/repos/install.go — PR body claims "zero behavioral change" and "Refactor-only" but the plan doc (docs/plans/repos-management.md) was already updated from "Zero behavioral change" to "Preserves install semantics", which is a weaker and more accurate claim. The actual code flow is equivalent (WIF provisioning completes before scaffold delivery in both old and new code), so this is a documentation wording issue rather than an actual behavioral change.
  Remediation: Update the PR body to say "Preserves install semantics" instead of "zero behavioral change" to match the plan doc.

Low

• [logic-error] internal/cli/admin.go:808 — The dry-run path constructs its own InstallConfig that diverges from the non-dry-run installCfg (omits Skip* flags, uses mintDisplay for MintURL). BuildScaffoldFiles verifiably reads only Owner, Repo, Roles, VendorBinary, UpstreamRef, UpstreamTag — and the code includes a comment documenting this — so the divergence is harmless today. If BuildScaffoldFiles is extended to read additional fields, the dry-run config would silently diverge.

• [testing-override-exposure] internal/cli/admin.go:163 — The testClient, testPrinter, and testWIFProvisioner fields on perRepoInstallConfig couple test infrastructure into the production struct. If testClient is set but testPrinter is nil, printer.Banner() will nil-pointer panic at runtime. All current tests set both via perRepoTestBase() which always initializes testPrinter, so this is not an active bug.

• [abstraction-alignment] internal/repos/install.go — The ScaffoldCommitFunc callback signature is a standard dependency injection pattern (repos defines the abstraction, CLI satisfies it). The inversion is architecturally correct but worth noting as a design choice that couples the Install() API to a specific callback shape.

• [naming-consistency] internal/cli/admin.go — The gcfWIFAdapter type name mixes the GCF package abbreviation with the WIF domain concept. For an adapter bridging repos.WIFProvisioner to gcf.Provisioner, a name like gcfProvisionerAdapter would more clearly convey the adapter's role.

Previous run (3)

Looks good to me

Findings

Low

• [logic-error] internal/cli/admin.go:808 — The dry-run path constructs its own InstallConfig that diverges from the non-dry-run installCfg (omits Skip* flags, uses mintDisplay for MintURL). BuildScaffoldFiles verifiably reads only Owner, Repo, Roles, VendorBinary, UpstreamRef, UpstreamTag — and the code includes a comment documenting this — so the divergence is harmless today. If BuildScaffoldFiles is extended to read additional fields, the dry-run config would silently diverge.

• [duplication-drift-risk] internal/repos/install.go:24 — The wifProviderPattern regex is duplicated between internal/repos/install.go and internal/cli/admin.go:164. Both compile the identical regex. If one is updated without the other, validation will diverge between the CLI input path and the install path.

• [testing-override-exposure] internal/cli/admin.go:163 — The testClient, testPrinter, and testWIFProvisioner fields on perRepoInstallConfig couple test infrastructure into the production struct. If testClient is set but testPrinter is nil, printer.Banner() will nil-pointer panic. All current tests set both, so this is not an active bug.

• [interface-contract-drift] internal/repos/install.go:102 — The WIFProvisioner interface defines five methods but Install() only calls DiscoverMint and ProvisionWIF. The other three methods exist for anticipated future callers (repos management plan PRs 5-8).

• [documentation] internal/maputil/maputil.go — Package maputil is missing a package-level doc comment. Other packages in internal/ consistently use package doc comments.
  Remediation: Add // Package maputil provides map utility functions.

Previous run (4)

Looks good to me

Findings

Low

• [logic-error] internal/cli/admin.go:808 — The dry-run path constructs its own InstallConfig that diverges from the non-dry-run installCfg (omits Skip* flags, uses mintDisplay for MintURL). BuildScaffoldFiles verifiably reads only Owner, Repo, Roles, VendorBinary, UpstreamRef, UpstreamTag — and the code includes a comment documenting this — so the divergence is harmless today. If BuildScaffoldFiles is extended to read additional fields, the dry-run config would silently diverge.

• [duplication-drift-risk] internal/repos/install.go:24 — The wifProviderPattern regex is duplicated between internal/repos/install.go and internal/cli/admin.go:164. Both compile the identical regex. If one is updated without the other, validation will diverge between the CLI input path and the install path.

• [testing-override-exposure] internal/cli/admin.go:163 — The testClient, testPrinter, and testWIFProvisioner fields on perRepoInstallConfig couple test infrastructure into the production struct. If testClient is set but testPrinter is nil, printer.Banner() will nil-pointer panic. All current tests set both, so this is not an active bug.

• [interface-contract-drift] internal/repos/install.go:102 — The WIFProvisioner interface defines five methods but Install() only calls DiscoverMint and ProvisionWIF. The other three methods exist for anticipated future callers (repos management plan PRs 5–8).

• [documentation] internal/maputil/maputil.go — Package maputil is missing a package-level doc comment. Other packages in internal/ consistently use package doc comments.
  Remediation: Add // Package maputil provides map utility functions.

Previous run (5)

Looks good to me

Findings

Low

• [behavioral-change-claim] internal/cli/admin.go:808 — The dry-run path constructs a standalone InstallConfig with 11 fields (Owner, Repo, Roles, MintURL, InferenceProject, InferenceRegion, UpstreamRef, UpstreamTag, WIFProvider, VendorBinary, Direct) while the non-dry-run installCfg carries 15+ fields including Skip* flags. Currently functionally equivalent since BuildScaffoldFiles only reads Owner, Repo, Roles, VendorBinary, UpstreamRef, UpstreamTag. If BuildScaffoldFiles is extended to depend on additional InstallConfig fields, the dry-run config would silently diverge.
  Remediation: Consider extracting the dry-run InstallConfig construction to share the same builder as the non-dry-run path, or add a comment noting the intentional subset.

• [interface-contract-drift] internal/repos/install.go — WIFProvisioner interface defines DeletePerRepoWIF(), RegisterPerRepoWIF(), and EnsureOrgInMint() which are not called by Install() in this PR. Forward-looking for future fullsend repos install/sync/remove commands (PRs 5-8 in the plan). Forces test fakes to stub unused methods.

• [testing-override-exposure] internal/cli/admin.go:163 — PR adds testClient, testPrinter, and testWIFProvisioner fields to perRepoInstallConfig. These allow bypassing production resolveToken() and real GCF provisioner. Fields are unexported and not settable via CLI flags, accessible only within the cli package. Couples test infrastructure into production code rather than using interfaces injected via constructor parameters.

• [api-surface] internal/maputil/maputil.go — Exported function SortedKeys lacks a godoc comment. Codebase convention for exported functions includes doc comments (e.g., urlutil.IsURL has // IsURL returns true if...).
  Remediation: Add a godoc comment, e.g.: // SortedKeys returns the keys of m in lexicographic order.

• [missing-authorization] N/A — No linked issue for this PR. The PR body references ADR 0057 (Accepted) and docs/plans/repos-management.md as authorization. ADR 0057 provides strategic authorization (intent authorization tier 2) for the repos management feature.
  Remediation: Consider linking a tracking issue for the ADR 0057 implementation plan.

Previous run (6)

Looks good to me

Findings

Low

• [behavioral-change-claim] internal/cli/admin.go:807 — The dry-run path calls repos.BuildScaffoldFiles() with a freshly constructed InstallConfig carrying only 6 fields (Owner, Repo, Roles, VendorBinary, UpstreamRef, UpstreamTag), while the non-dry-run installCfg has 15+ fields. Currently functionally equivalent since BuildScaffoldFiles only reads those 6 fields, but if it were extended to depend on additional fields, the inline config would silently diverge.

• [interface-contract-drift] internal/repos/install.go:127 — WIFProvisioner interface defines DeletePerRepoWIF(), RegisterPerRepoWIF(), and EnsureOrgInMint() which are not called by Install() in this PR. These are forward-looking for the future fullsend repos install command but force test fakes to stub unused methods.

• [semantic-preservation-claim] internal/cli/admin.go:961 — PR claims "Preserves install semantics" — the vendor path transaction boundaries are equivalent between old and new code (WIF first, then scaffold+vars+secrets via applyPerRepoScaffold), but the intermediate flow differs: repos.Install() with SkipScaffoldAndConfig=true returns early, then applyPerRepoScaffold handles the rest. Functionally equivalent but worth noting the structural change.

• [code-duplication] internal/repos/install.go — sortedStringMapKeys is duplicated across 3+ files: internal/cli/admin.go, internal/dispatch/gcf/provisioner.go, and internal/repos/install.go. The function is trivial (7 lines) but three copies suggests extraction to a shared utility.
  Remediation: Extract to a shared utility package (e.g., internal/sliceutil or internal/maputil).

Previous run (7)

Review

Findings

Low

• [error-context-loss] internal/cli/admin.go:280 — gcfWIFAdapter.DiscoverMint wraps gcf.ErrFunctionNotFound into repos.ErrMintNotFound using fmt.Errorf("%w", repos.ErrMintNotFound), which discards the original error chain. The upstream gcf error may contain context such as the project/region that was queried. A caller doing errors.Is(err, repos.ErrMintNotFound) will succeed, but diagnostic logging will lose the underlying cause.
  Remediation: Consider fmt.Errorf("mint function not found: %w", repos.ErrMintNotFound) or a multi-error wrapper that preserves both.

• [behavioral-change-claim] internal/cli/admin.go — The PR description claims "zero behavioral change" but the vendor mode path now calls repos.BuildScaffoldFiles() after repos.Install() returns, recomputing scaffold files at a different point compared to the original code where files was computed once before the vendor/non-vendor branch. The dry-run path also constructs a partial InstallConfig for BuildScaffoldFiles rather than using pre-computed files. These are functionally equivalent but the claim is technically inaccurate.

• [interface-contract-drift] internal/repos/install.go — The WIFProvisioner interface defines DeletePerRepoWIF(), RegisterPerRepoWIF(), and EnsureOrgInMint() which are not called by Install() in this PR. This is a reasonable forward-looking design for the 8-PR plan, but it forces test fakes to stub unused methods.

• [code-duplication] internal/repos/install.go — sortedStringMapKeys is duplicated across 3 files: internal/cli/admin.go, internal/dispatch/gcf/provisioner.go, and internal/repos/install.go. The function is trivial (7 lines) but three copies across separate packages suggests extraction to a shared utility.
  Remediation: Extract to a shared utility package (e.g., internal/sliceutil or internal/maputil).

• [stale-reference] docs/plans/repos-management.md — Plan document still describes PR 1 as future work rather than completed work. This is standard practice for in-progress multi-PR series and can be updated when the series completes.
  Remediation: Update the plan document to reflect that PR 1 has been implemented once the series is further along.

• [stale-reference] docs/guides/dev/cli-internals.md:270 — Reference to runPerRepoInstall() as the current implementation location. While this function still exists, it now delegates to the extracted repos.Install() function.
  Remediation: Add a note that runPerRepoInstall() is now a thin wrapper around repos.Install() from the internal/repos package.

• [stale-reference] docs/guides/dev/cli-internals.md:246 — The table shows scaffold.PerRepoCustomizedDirs() / WalkFullsendRepo() as the scaffold source for Phase 5, but the new implementation uses repos.BuildScaffoldFiles() which internally calls scaffold.CollectPerRepoInstallFiles().
  Remediation: Update the table to reference the new code path.

Previous run (8)

Review

Findings

Low

• [code-duplication] internal/repos/install.go — sortedStringMapKeys is duplicated across 3 files: internal/cli/admin.go, internal/dispatch/gcf/provisioner.go, and internal/repos/install.go. The function is trivial (7 lines) but three copies across separate packages suggests extraction to a shared utility.
  Remediation: Extract to a shared utility package (e.g., internal/sliceutil or internal/maputil).

• [behavioral-change-claim] internal/cli/admin.go — The PR description claims "zero behavioral change" but the vendor mode path now calls repos.BuildScaffoldFiles() after repos.Install() returns, recomputing scaffold files at a different point compared to the original code where files was computed once before the vendor/non-vendor branch. The dry-run path also constructs a partial InstallConfig for BuildScaffoldFiles rather than using pre-computed files. These are functionally equivalent but the claim is technically inaccurate.

• [interface-contract-drift] internal/repos/install.go — The WIFProvisioner interface defines DeletePerRepoWIF(), RegisterPerRepoWIF(), and EnsureOrgInMint() which are not called by Install() in this PR. This is a reasonable forward-looking design for the 8-PR plan, but it forces test fakes to stub unused methods.

• [secrets-handling] internal/repos/install.go — The FULLSEND_GCP_WIF_PROVIDER secret value flows from InstallConfig.WIFProvider without re-validation against the wifProviderPattern regex. Current call sites are safe (admin.go validates before constructing InstallConfig), but future callers could pass an unvalidated value.

• [naming-alignment] internal/repos/install.go — Package named repos (plural) while the dominant codebase convention uses singular names (config, forge, scaffold, dispatch). layers is the only counterexample.

---

Labels: PR is a refactor extracting install logic into a new package — type/chore applies

Previous run (9)

Review

Findings

Low

• [interface-contract-drift] internal/repos/install.go — The WIFProvisioner interface defines DeletePerRepoWIF(), RegisterPerRepoWIF(), and EnsureOrgInMint() which are not called by Install() in this PR. The gcfWIFAdapter in admin.go implements all five methods but only two have callers. This is a reasonable forward-looking design for the 8-PR plan (the methods have concrete implementations and the interface serves as the abstraction boundary for the entire repos package), but it does force test fakes to stub unused methods. See also: [unused-interface-methods] finding at this location.

• [authorization-scope-alignment] N/A — This PR has no linked issue. Non-trivial changes (1255 changed lines) typically require explicit authorization via a linked issue. However, this is PR 1 of the 8-PR implementation plan documented in docs/plans/repos-management.md, and ADR 0057 (Accepted) authorizes the overall feature.
  Remediation: Consider adding a tracking issue for the ADR 0057 implementation plan.

• [behavioral-change-claim] internal/cli/admin.go — The PR description claims "Zero behavioral change" but the vendor mode path now calls repos.BuildScaffoldFiles() before appendVendorTreeFiles(), recomputing scaffold files at a different point compared to the original code where files was computed once before the vendor/non-vendor branch. The dry-run path also now calls repos.BuildScaffoldFiles() inline rather than using pre-computed files. These are functionally equivalent but the "zero behavioral change" claim is technically inaccurate.
  Remediation: Revise PR description to "Preserves install semantics" or document any observable differences.

• [code-duplication] internal/repos/install.go — sortedStringMapKeys is now duplicated across 3 files: internal/cli/admin.go, internal/dispatch/gcf/provisioner.go, and internal/repos/install.go. The function is trivial (7 lines) but three copies across separate packages suggests extraction to a shared utility.
  Remediation: Extract to a shared utility package.

• [missing-documentation] docs/guides/dev/cli-internals.md — The CLI Internals documentation references internal/scaffold/ but does not mention the new internal/repos package with its exported types (InstallConfig, InstallResult, WIFProvisioner, Install, BuildScaffoldFiles). Low priority since the package may evolve further across the remaining PRs.
  Remediation: Add a section documenting the internal/repos package and its role in the installation architecture.

---

Labels: Refactoring PR extracting install logic into a new package is maintenance/housekeeping work.

Previous run (10)

Review

Findings

Low

• [missing-authorization] N/A — This PR adds 1211 lines (3 files modified/created) implementing ADR 0057 PR 1, but has no linked issue. Non-trivial changes require explicit authorization via a linked issue that establishes scope and intent. Downgraded from high because this is a pure code extraction refactor with documented authorization via ADR 0057 and the implementation plan at docs/plans/repos-management.md.
  Remediation: Link this PR to a tracking issue for ADR 0057 implementation (or create one if it doesn't exist).

• [api-contract-violation] internal/repos/install.go — When SkipWIF=true and WIFProvider is empty and SkipScaffoldAndConfig=false, the install proceeds and writes an empty string as the FULLSEND_GCP_WIF_PROVIDER secret. The current admin.go caller cannot trigger this (because SkipWIF is only true when inferenceWIFProvider is non-empty), but future callers of repos.Install could hit this path.
  Remediation: Consider adding a validation: if SkipWIF is false and WIFProvider is empty and !SkipScaffoldAndConfig, return an error rather than writing an empty secret.

• [edge-case] internal/repos/install.go — When SkipMintCheck=false and MintURL is empty, mint discovery runs. If provisioner.DiscoverMint() returns a MintDiscovery with an empty URL and no error, the install proceeds and writes an empty FULLSEND_MINT_URL variable. Pre-existing behavior faithfully reproduced in the extraction.
  Remediation: Add a post-discovery validation that discovery.URL is non-empty.

• [test-coverage] internal/repos/install_test.go — No test covers the edge case where SkipWIF=false, WIFProvider="", and SkipScaffoldAndConfig=false to verify that an empty WIF provider value is (or is not) rejected before being written as a secret.

Previous run (11)

Review

Findings

Medium

• [interface-coherence] internal/cli/admin.go — The gcfWIFAdapter.DiscoverMint implementation violates the WIFProvisioner interface's error contract. The interface documents that DiscoverMint wraps ErrMintNotFound when the mint function does not exist. The adapter returns repos.ErrMintNotFound when provisioner is nil, but passes through gcf.ErrFunctionNotFound unchanged when a.provisioner.DiscoverMint fails. Callers checking errors.Is(err, repos.ErrMintNotFound) will not match when the GCF provisioner returns its sentinel error.
  Remediation: In gcfWIFAdapter.DiscoverMint, translate the GCF error: if errors.Is(err, gcf.ErrFunctionNotFound) { return nil, fmt.Errorf("%w", repos.ErrMintNotFound) }. Otherwise return the original error.

Low

• [api-contract-violation] internal/repos/install.go — When SkipWIF=true and WIFProvider is empty and SkipScaffoldAndConfig=false, an empty FULLSEND_GCP_WIF_PROVIDER secret gets written. The current admin.go caller cannot trigger this because SkipWIF and WIFProvider are derived from the same variable (needsWIFProvision), but a future bulk-install caller could.
  Remediation: Document the invariant in InstallConfig's godoc that when SkipWIF is false, either WIFProvider must be non-empty or a provisioner must be provided.

• [scope-coherence] internal/cli/admin.go — The vendor code path in runPerRepoInstall continues to diverge from the non-vendor path. When vendor=true: repos.Install is called with SkipScaffoldAndConfig=true and returns early, then admin.go rebuilds scaffold files via repos.BuildScaffoldFiles, appends vendor files, and calls applyPerRepoScaffold with inline var/secret definitions. When vendor=false: repos.Install handles everything internally. The split is documented but leaves future fullsend repos install with two implementations.

• [edge-case] internal/repos/install.go — When SkipMintCheck=false and MintURL is empty, mint discovery runs. If provisioner.DiscoverMint() returns a MintDiscovery with an empty URL and no error, the install proceeds and writes an empty FULLSEND_MINT_URL variable. Not a regression (old code had same gap) but a concern for the new reusable function.

• [test-coverage] internal/repos/install_test.go — No test for SkipWIF=false with WIFProvider already non-empty (pre-provisioned). In Install() the condition is !cfg.SkipWIF && wifProvider == "", so SkipWIF=false but WIFProvider pre-set silently skips provisioning. This interaction is subtle enough to warrant a test.

• [naming-coherence] internal/repos/install.go — Sentinel error ErrMintNotFound uses fmt.Errorf instead of errors.New, diverging from codebase pattern where other sentinel errors use errors.New (forge.ErrNotFound, forge.ErrAlreadyExists, gcf.ErrFunctionNotFound).

• [code-duplication] internal/repos/install.go — sortedStringMapKeys helper duplicated across 3 files: internal/cli/admin.go, internal/dispatch/gcf/provisioner.go, and internal/repos/install.go.

Previous run (12)

Review

Findings

Medium

• [api-contract-violation] internal/cli/admin.go — The scaffoldCommitFn closure always returns ("", err), discarding the PR URL. layers.CommitScaffoldFiles returns (bool, error), not a URL string, so result.ScaffoldPR will always be empty in the real implementation even when a PR was created. The current revision documents this gap in the ScaffoldCommitFunc and InstallResult.ScaffoldPR godoc comments, making the limitation transparent rather than hidden — but the contract remains unfulfilled. Any future consumer relying on ScaffoldPR being non-empty will silently get an empty string.
  Remediation: Either plumb the PR URL through layers.CommitScaffoldFiles (change its return from (bool, error) to include the URL) or remove the ScaffoldPR field from InstallResult until the plumbing exists. Create a tracking issue for this debt.

• [scope-coherence] internal/cli/admin.go — The vendor code path still calls applyPerRepoScaffold after repos.Install returns (with SkipScaffoldAndConfig: vendor), creating two divergent code paths: non-vendor goes through repos.Install for scaffold + vars + secrets, while vendor rebuilds scaffold files via repos.BuildScaffoldFiles, appends vendor tree files, and calls applyPerRepoScaffold separately. This is intentional (vendor needs atomic scaffold+vendor commit) and documented with a comment, but it means the vendor path cannot be reused by the future fullsend repos install command without also extracting applyPerRepoScaffold.
  Remediation: Consider unifying in a follow-up PR by adding a ScaffoldFilesTransform callback or accepting pre-built files in repos.Install, so both paths flow through the same function.

Low

• [missing-authorization] N/A — PR has no linked issue. The PR body references ADR 0057 but does not link to it or to a tracking issue. Adding a reference (e.g., "Implements ADR 0057" or "Relates to #NNN") would help reviewers trace authorization.

• [abstraction-leakage] internal/repos/install.go — The repos.WIFProvisioner interface mirrors gcf.Provisioner methods closely. The gcfWIFAdapter in admin.go is a thin wrapper, though it does perform data mapping (gcf.MintDiscovery → repos.MintDiscovery) and method renaming (RemoveRepoFromMint → DeletePerRepoWIF), which provides genuine decoupling.

• [comment-consistency] internal/repos/install.go — The WIFProvisioner interface comment uses Go 1.19+ doc link bracket notation ([errors.Is], [DiscoverMint], [ErrMintNotFound]) which is not used elsewhere in this codebase. Consider using plain text references for consistency.

• [code-duplication] internal/repos/install.go — sortedStringMapKeys is duplicated in three files: internal/cli/admin.go, internal/dispatch/gcf/provisioner.go, and internal/repos/install.go. The function is trivial (7 lines) and unexported, so sharing requires exporting from a common package.

• [import-grouping] internal/cli/admin.go — Extra blank line after the repos import creates a fourth import group. The existing file uses three groups: stdlib, third-party, internal.

• [naming-alignment] internal/repos/install.go — Package named repos (plural) while the dominant codebase convention uses singular names (config, forge, scaffold). layers is a counterexample, so the convention is not absolute.

• [variable-naming] internal/cli/admin.go — Variable name wifProv uses abbreviation while the codebase prefers full words for non-standard abbreviations (e.g., provisioner, mintProvisioner).

• [test-helper-naming] internal/repos/install_test.go — fakeWIFProvisioner follows the forge.FakeClient convention. Both fake and mock prefixes exist in the codebase; fake is consistent with the imported forge.FakeClient used in the same file.

Previous run

Review

Findings

Medium

• [api-contract-violation] internal/cli/admin.go — The scaffoldCommitFn closure always returns ("", err), discarding the PR URL. layers.CommitScaffoldFiles returns (bool, error), not a URL, so the closure can never fulfil the ScaffoldCommitFunc contract which declares a prURL string return. repos.Install stores this in result.ScaffoldPR, which will always be empty even when a PR was created. The test TestInstall_ScaffoldCommitReturnsURL passes only because the test fake directly returns a URL, masking the problem in the real implementation.
  Remediation: Either plumb the PR URL through layers.CommitScaffoldFiles (may need a small change to return the URL), or document on ScaffoldCommitFunc that prURL may be empty, or remove ScaffoldPR from InstallResult until the plumbing is in place.

Low

• [nil-dereference] internal/repos/install.go:151 — The progress callback (ProgressFunc) is called unconditionally at ~15 call sites throughout Install with no nil guard. If any future caller passes nil, the function panics. The current caller in admin.go always provides a non-nil function, so this is not an active bug, but a defensive-programming gap in the exported API.
  Remediation: Add a nil guard at the top of Install: if progress == nil { progress = func(_, _, _ string) {} }.

• [edge-case] internal/repos/install.go:166 — When SkipMintCheck is false and MintURL is empty but provisioner is nil, mint discovery is silently skipped. The function then writes FULLSEND_MINT_URL as an empty string to repo variables. The current caller sets SkipMintCheck=true so this path is unreachable today, but future bulk-install callers could hit it.

• [edge-case] internal/repos/install.go:177 — Same pattern for WIF: when SkipWIF is false, WIFProvider is empty, but provisioner is nil, WIF provisioning is silently skipped and FULLSEND_GCP_WIF_PROVIDER is written as an empty string to repo secrets.
  Remediation: Validate that mintURL and wifProvider are non-empty before writing them as repo variables/secrets, or return an error when provisioner is nil but required.

• [code-duplication] internal/repos/install.go:306 — sortedStringMapKeys is now duplicated in three locations: internal/cli/admin.go:2780, internal/dispatch/gcf/provisioner.go:1576, and the new internal/repos/install.go. The function is trivial (7 lines), so duplication cost is minimal, but three copies suggests extraction to a shared utility.

---

Labels: Refactoring PR extracting install logic into a new internal package — existing component/install and go labels are appropriate.

Previous run (13)

Review

Findings

Low

• [logic-error] internal/cli/admin.go:675 — Dead code: scaffold files are built at lines 659-687 (config validation, YAML marshaling, CollectPerRepoInstallFiles, TreeFile assembly) but this files slice is never used in the non-vendor path (repos.Install builds its own files internally via BuildScaffoldFiles) and is overwritten in the vendor path (line 1078: files = scaffoldFiles). The config validation and scaffold collection at lines 659-672 run unnecessarily since repos.Install and BuildScaffoldFiles perform the same work.

• [logic-error] internal/repos/install.go:29 — InstallConfig declares MintProject and MintRegion fields that are never read anywhere in the repos package. The sole caller in admin.go does not populate these fields either, making them dead struct fields that could mislead future callers into thinking they affect behavior.

• [naming-convention] internal/repos/install.go:308 — Helper function sortedKeys uses a different name from the established codebase pattern sortedStringMapKeys, which appears in internal/cli/admin.go, internal/cli/github.go, and internal/dispatch/gcf/provisioner.go for the identical function signature and behavior.

---

Labels: Refactoring PR extracting install logic into a new internal package — type/chore fits the maintenance nature of the change.

Previous run (14)

Review

Findings

High

• [logic-error] internal/cli/admin.go — In the vendor path, repos.Install() is called first (which commits scaffold files AND writes vars+secrets via steps 4-7 in install.go), and then the if vendor block rebuilds scaffold files via BuildScaffoldFiles, appends vendor tree files, rebuilds the same repoVars and repoSecrets maps, and calls applyPerRepoScaffold() which commits scaffold files AND writes vars+secrets a second time. This produces two commits and two rounds of var/secret API writes for every vendor install. The old code had a single call to applyPerRepoScaffold with the vendor-appended files. The first commit from repos.Install will contain only base scaffold files (without vendored assets), creating a broken intermediate state.
  Remediation: Skip scaffold commit and var/secret writes inside repos.Install() for the vendor path (e.g., add a SkipScaffold flag to InstallConfig), or remove the applyPerRepoScaffold call from the vendor block and instead pass vendor files into repos.Install().

• [api-contract] internal/repos/install.go — The new commitScaffold() function uses client.CommitFiles/client.CommitFilesToBranch directly, while the old code path used layers.CommitScaffoldFiles() which provides: fork detection and cross-fork PR creation for non-owner users, interactive fork-vs-upstream prompts, branch protection fallback (direct mode falls back to PR when branch is protected), and non-fast-forward retry logic. By bypassing layers.CommitScaffoldFiles, the new code will fail for non-owner users who need fork-based PRs and will not gracefully handle branch protection.
  Remediation: Have commitScaffold() delegate to layers.CommitScaffoldFiles() rather than calling forge client methods directly.

Medium

• [stale-reference] internal/cli/admin.go — WIFProvider field in installCfg is set to inferenceWIFProvider (which is empty at that point since WIF provisioning was moved into repos.Install()). The result is read back via installResult.WIFProvider after the call and inferenceWIFProvider is updated. While the flow works, it is fragile — if installResult.WIFProvider is empty, the vendor path writes an empty FULLSEND_GCP_WIF_PROVIDER secret.

• [error-handling-idiom] internal/repos/install.go — Guard check error handling logs the error via progress callback but continues installation. This is an intentional design for bulk-install resilience (tested by TestInstall_GuardCheckError_ContinuesInstall), but the rationale should be documented in a code comment explaining why this deviates from the codebase's fail-fast convention.

• [naming-convention] internal/repos/install.go — MintDiscovery struct is named inconsistently with some codebase patterns. However, since the repos-management plan itself uses this name, this is a minor concern.

Low

• [dead-code] internal/cli/admin.go — gcfWIFAdapter.discoverer field is never populated in this PR. Since SkipMintCheck=true, DiscoverMint is intentionally unreachable from this call path, but the unused field may confuse future maintainers.

• [error-handling-idiom] internal/repos/install.go — WIFProvisioner interface methods could benefit from more thorough error documentation beyond ErrMintNotFound for DiscoverMint.

• [doc-style] internal/cli/admin.go — gcfWIFAdapter comment says "wraps a gcf.Provisioner" (singular) but the struct has two *gcf.Provisioner fields (discoverer and wifProvisioner).

• [naming-convention] internal/repos/install.go — BuildScaffoldFiles() is exported; the comment documents the rationale (dry-run path), but the only external usage in this diff is the vendor path in admin.go.

• [naming-convention] internal/repos/install.go — InstallConfig uses inline comments for field documentation; consider grouping related Skip* flags with a block comment.

---

Labels: PR extracts per-repo install logic into a new Go package under internal/repos/.

Previous run (15)

Review

Findings

Medium

• [api-contract-violation] internal/cli/admin.go — The scaffoldCommitFn closure always returns ("", err), discarding the PR URL. layers.CommitScaffoldFiles returns (bool, error), not a URL string, so result.ScaffoldPR will always be empty in the real implementation even when a PR was created. The current revision documents this gap in the ScaffoldCommitFunc and InstallResult.ScaffoldPR godoc comments, making the limitation transparent rather than hidden — but the contract remains unfulfilled. Any future consumer relying on ScaffoldPR being non-empty will silently get an empty string.
  Remediation: Either plumb the PR URL through layers.CommitScaffoldFiles (change its return from (bool, error) to include the URL) or remove the ScaffoldPR field from InstallResult until the plumbing exists. Create a tracking issue for this debt.

• [scope-coherence] internal/cli/admin.go — The vendor code path still calls applyPerRepoScaffold after repos.Install returns (with SkipScaffoldAndConfig: vendor), creating two divergent code paths: non-vendor goes through repos.Install for scaffold + vars + secrets, while vendor rebuilds scaffold files via repos.BuildScaffoldFiles, appends vendor tree files, and calls applyPerRepoScaffold separately. This is intentional (vendor needs atomic scaffold+vendor commit) and documented with a comment, but it means the vendor path cannot be reused by the future fullsend repos install command without also extracting applyPerRepoScaffold.
  Remediation: Consider unifying in a follow-up PR by adding a ScaffoldFilesTransform callback or accepting pre-built files in repos.Install, so both paths flow through the same function.

Low

• [missing-authorization] N/A — PR has no linked issue. The PR body references ADR 0057 but does not link to it or to a tracking issue. Adding a reference (e.g., "Implements ADR 0057" or "Relates to #NNN") would help reviewers trace authorization.

• [abstraction-leakage] internal/repos/install.go — The repos.WIFProvisioner interface mirrors gcf.Provisioner methods closely. The gcfWIFAdapter in admin.go is a thin wrapper, though it does perform data mapping (gcf.MintDiscovery → repos.MintDiscovery) and method renaming (RemoveRepoFromMint → DeletePerRepoWIF), which provides genuine decoupling.

• [comment-consistency] internal/repos/install.go — The WIFProvisioner interface comment uses Go 1.19+ doc link bracket notation ([errors.Is], [DiscoverMint], [ErrMintNotFound]) which is not used elsewhere in this codebase. Consider using plain text references for consistency.

• [code-duplication] internal/repos/install.go — sortedStringMapKeys is duplicated in three files: internal/cli/admin.go, internal/dispatch/gcf/provisioner.go, and internal/repos/install.go. The function is trivial (7 lines) and unexported, so sharing requires exporting from a common package.

• [import-grouping] internal/cli/admin.go — Extra blank line after the repos import creates a fourth import group. The existing file uses three groups: stdlib, third-party, internal.

• [naming-alignment] internal/repos/install.go — Package named repos (plural) while the dominant codebase convention uses singular names (config, forge, scaffold). layers is a counterexample, so the convention is not absolute.

• [variable-naming] internal/cli/admin.go — Variable name wifProv uses abbreviation while the codebase prefers full words for non-standard abbreviations (e.g., provisioner, mintProvisioner).

• [test-helper-naming] internal/repos/install_test.go — fakeWIFProvisioner follows the forge.FakeClient convention. Both fake and mock prefixes exist in the codebase; fake is consistent with the imported forge.FakeClient used in the same file.

Previous run (16)

Review

Findings

Medium

• [api-contract-violation] internal/cli/admin.go — The scaffoldCommitFn closure always returns ("", err), discarding the PR URL. layers.CommitScaffoldFiles returns (bool, error), not a URL, so the closure can never fulfil the ScaffoldCommitFunc contract which declares a prURL string return. repos.Install stores this in result.ScaffoldPR, which will always be empty even when a PR was created. The test TestInstall_ScaffoldCommitReturnsURL passes only because the test fake directly returns a URL, masking the problem in the real implementation.
  Remediation: Either plumb the PR URL through layers.CommitScaffoldFiles (may need a small change to return the URL), or document on ScaffoldCommitFunc that prURL may be empty, or remove ScaffoldPR from InstallResult until the plumbing is in place.

Low

• [nil-dereference] internal/repos/install.go:151 — The progress callback (ProgressFunc) is called unconditionally at ~15 call sites throughout Install with no nil guard. If any future caller passes nil, the function panics. The current caller in admin.go always provides a non-nil function, so this is not an active bug, but a defensive-programming gap in the exported API.
  Remediation: Add a nil guard at the top of Install: if progress == nil { progress = func(_, _, _ string) {} }.

• [edge-case] internal/repos/install.go:166 — When SkipMintCheck is false and MintURL is empty but provisioner is nil, mint discovery is silently skipped. The function then writes FULLSEND_MINT_URL as an empty string to repo variables. The current caller sets SkipMintCheck=true so this path is unreachable today, but future bulk-install callers could hit it.

• [edge-case] internal/repos/install.go:177 — Same pattern for WIF: when SkipWIF is false, WIFProvider is empty, but provisioner is nil, WIF provisioning is silently skipped and FULLSEND_GCP_WIF_PROVIDER is written as an empty string to repo secrets.
  Remediation: Validate that mintURL and wifProvider are non-empty before writing them as repo variables/secrets, or return an error when provisioner is nil but required.

• [code-duplication] internal/repos/install.go:306 — sortedStringMapKeys is now duplicated in three locations: internal/cli/admin.go:2780, internal/dispatch/gcf/provisioner.go:1576, and the new internal/repos/install.go. The function is trivial (7 lines), so duplication cost is minimal, but three copies suggests extraction to a shared utility.

---

Labels: Refactoring PR extracting install logic into a new internal package — existing component/install and go labels are appropriate.

Previous run (17)

Review

Findings

Low

• [logic-error] internal/cli/admin.go:675 — Dead code: scaffold files are built at lines 659-687 (config validation, YAML marshaling, CollectPerRepoInstallFiles, TreeFile assembly) but this files slice is never used in the non-vendor path (repos.Install builds its own files internally via BuildScaffoldFiles) and is overwritten in the vendor path (line 1078: files = scaffoldFiles). The config validation and scaffold collection at lines 659-672 run unnecessarily since repos.Install and BuildScaffoldFiles perform the same work.

• [logic-error] internal/repos/install.go:29 — InstallConfig declares MintProject and MintRegion fields that are never read anywhere in the repos package. The sole caller in admin.go does not populate these fields either, making them dead struct fields that could mislead future callers into thinking they affect behavior.

• [naming-convention] internal/repos/install.go:308 — Helper function sortedKeys uses a different name from the established codebase pattern sortedStringMapKeys, which appears in internal/cli/admin.go, internal/cli/github.go, and internal/dispatch/gcf/provisioner.go for the identical function signature and behavior.

---

Labels: Refactoring PR extracting install logic into a new internal package — type/chore fits the maintenance nature of the change.

Previous run (18)

Review

Findings

High

• [logic-error] internal/cli/admin.go — In the vendor path, repos.Install() is called first (which commits scaffold files AND writes vars+secrets via steps 4-7 in install.go), and then the if vendor block rebuilds scaffold files via BuildScaffoldFiles, appends vendor tree files, rebuilds the same repoVars and repoSecrets maps, and calls applyPerRepoScaffold() which commits scaffold files AND writes vars+secrets a second time. This produces two commits and two rounds of var/secret API writes for every vendor install. The old code had a single call to applyPerRepoScaffold with the vendor-appended files. The first commit from repos.Install will contain only base scaffold files (without vendored assets), creating a broken intermediate state.
  Remediation: Skip scaffold commit and var/secret writes inside repos.Install() for the vendor path (e.g., add a SkipScaffold flag to InstallConfig), or remove the applyPerRepoScaffold call from the vendor block and instead pass vendor files into repos.Install().

• [api-contract] internal/repos/install.go — The new commitScaffold() function uses client.CommitFiles/client.CommitFilesToBranch directly, while the old code path used layers.CommitScaffoldFiles() which provides: fork detection and cross-fork PR creation for non-owner users, interactive fork-vs-upstream prompts, branch protection fallback (direct mode falls back to PR when branch is protected), and non-fast-forward retry logic. By bypassing layers.CommitScaffoldFiles, the new code will fail for non-owner users who need fork-based PRs and will not gracefully handle branch protection.
  Remediation: Have commitScaffold() delegate to layers.CommitScaffoldFiles() rather than calling forge client methods directly.

Medium

• [stale-reference] internal/cli/admin.go — WIFProvider field in installCfg is set to inferenceWIFProvider (which is empty at that point since WIF provisioning was moved into repos.Install()). The result is read back via installResult.WIFProvider after the call and inferenceWIFProvider is updated. While the flow works, it is fragile — if installResult.WIFProvider is empty, the vendor path writes an empty FULLSEND_GCP_WIF_PROVIDER secret.

• [error-handling-idiom] internal/repos/install.go — Guard check error handling logs the error via progress callback but continues installation. This is an intentional design for bulk-install resilience (tested by TestInstall_GuardCheckError_ContinuesInstall), but the rationale should be documented in a code comment explaining why this deviates from the codebase's fail-fast convention.

• [naming-convention] internal/repos/install.go — MintDiscovery struct is named inconsistently with some codebase patterns. However, since the repos-management plan itself uses this name, this is a minor concern.

Low

• [dead-code] internal/cli/admin.go — gcfWIFAdapter.discoverer field is never populated in this PR. Since SkipMintCheck=true, DiscoverMint is intentionally unreachable from this call path, but the unused field may confuse future maintainers.

• [error-handling-idiom] internal/repos/install.go — WIFProvisioner interface methods could benefit from more thorough error documentation beyond ErrMintNotFound for DiscoverMint.

• [doc-style] internal/cli/admin.go — gcfWIFAdapter comment says "wraps a gcf.Provisioner" (singular) but the struct has two *gcf.Provisioner fields (discoverer and wifProvisioner).

• [naming-convention] internal/repos/install.go — BuildScaffoldFiles() is exported; the comment documents the rationale (dry-run path), but the only external usage in this diff is the vendor path in admin.go.

• [naming-convention] internal/repos/install.go — InstallConfig uses inline comments for field documentation; consider grouping related Skip* flags with a block comment.

---

Labels: PR extracts per-repo install logic into a new Go package under internal/repos/.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 12:39 AM UTC · Completed 12:50 AM UTC
Commit: 2fb19a8 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 12:57 AM UTC · Ended 1:05 AM UTC
Commit: 0a95cac · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:05 AM UTC · Completed 1:17 AM UTC
Commit: 3bbbea0 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 1:25 AM UTC · Completed 1:37 AM UTC
Commit: 9763b23 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:44 AM UTC · Completed 2:01 AM UTC
Commit: 5811df8 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:40 PM UTC · Completed 4:52 PM UTC
Commit: ddf49ce · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:55 PM UTC · Completed 5:11 PM UTC
Commit: f93ffd5 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:38 PM UTC · Completed 11:52 PM UTC
Commit: 9a6b620 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 11:55 PM UTC · Completed 12:17 AM UTC
Commit: a2d225f · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:49 AM UTC · Completed 12:57 AM UTC
Commit: a2d225f · View workflow run →

[fullsend-ai-retro]

PR #3003 extracted per-repo install logic into a new internal/repos package (1,685 additions, 10 files). The review agent was highly effective, catching 5+ real bugs across multiple categories: nil-dereference risk, guard check failing open, WIF error dropping results, empty secret writes, and an unused struct field. The human reviewer (ifireball) approved with zero findings, meaning the review agent strictly exceeded human review quality on this change.

However, the review cycle was highly inefficient: 7 review rounds produced 30 review submissions and 17 status comments over ~24 hours. The agent correctly approved in round 1 (03:47 UTC July 4), but continued posting findings through round 7 (23:51 UTC July 5). Rounds 4-7 repeatedly re-raised acknowledged findings (dry-run config divergence, test override exposure) that the author explicitly responded to but chose not to change. This represents significant wasted tokens and contributor attention.

No novel proposals are warranted because the identified inefficiencies are comprehensively covered by existing open issues:

• Finding deduplication across iterations: Review agent should deduplicate findings across iterations on the same PR #1013, Review agent should deduplicate findings across re-review iterations on the same PR #2959, Review agent: persistent finding cache for cross-iteration deduplication and dismissal enforcement #2816
• Honoring author dismissals: Review agent should honor explicit author dismissals of findings #1672, Review agent should not re-request changes for unchanged findings #1500
• Diminishing findings after approval: Review agent posts diminishing-value findings after approval #3025
• Review dispatch caps/circuit breakers: Add circuit breaker: cap review iterations per PR when no human engagement #2992, Add per-PR review budget to cap runaway review dispatches #2599, Add hard cap on total review dispatches per PR as a backstop #2587
• Status comment consolidation: Use comment updates with collapsible sections for review completion (supersedes #584) #971, Review status comment shows Terminated then posts a second Success comment for the same run #2572
• Autonomy tracking for internal Go packages: Track agent-vs-human review delta for internal Go packages to evaluate autonomy #3026

This PR provides strong additional evidence for #3026 (autonomy readiness for internal Go refactoring) and #3025 (post-approval finding waste). The review agent's bug-catching capability is mature; the iteration control mechanisms are the bottleneck.