feat(repos)!: add, remove, install, uninstall subcommands

[ggallen]

Closes #4098

Implements ADR-0057 PR 8: repos management subcommands for per-repo installations.

Summary

• repos add — add repo entries to the manifest (with optional --install)
• repos remove — remove repo entries from the manifest (with optional --uninstall)
• repos uninstall — tear down fullsend infrastructure from repos
• repos install — updated to accept positional args instead of --repo flag

BREAKING CHANGE: repos install --repo replaced by positional arguments. This is all new functionality that no users are yet using, so although it is technically a breaking change it will have zero user impact.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:51 PM UTC · Completed 10:03 PM UTC
Commit: 9e2b73d · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Add fullsend repos remove to uninstall fullsend from selected repos

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add repos remove CLI command with dry-run, concurrency, and optional WIF cleanup.
• Implement two-phase uninstall: parallel repo cleanup then sequential WIF deregistration.
• Extend WIF deletion to remove the provider from the inference GCP project.

Diagram

graph TD
  A["CLI: repos remove"] --> B["repos.Remove()"] --> C["Phase 1: repo cleanup"] --> D{{"Forge API (GitHub)"}} --> E["Delete workflow + vars + secrets"]
  B --> F["Phase 2: WIF cleanup"] --> G["splitProjectAdapter"] --> H{{"Mint + GCP WIF"}}
  F --> I["repos.yaml manifest"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Batch mint deregistration update (single RMW)

• ➕ Reduces mint env var read/modify/write operations from N to 1
• ➕ Allows WIF provider deletions to proceed independently/possibly in parallel
• ➖ More complex error handling (need to reconcile per-repo failures into a single update)
• ➖ Harder to make progress reporting granular per repo
• ➖ Higher risk of incorrect mint state if computation/merge logic is wrong

2. Parallel WIF provider deletion + serialized mint updates (mutex)

• ➕ Faster overall WIF cleanup for many repos
• ➕ Keeps mint RMW correctness while improving throughput
• ➖ More concurrency complexity (two different critical sections)
• ➖ Still must define ordering/rollback semantics when some deletions fail

Recommendation: The PR’s approach (parallel repo cleanup, then sequential WIF cleanup) is the safest default because mint deregistration is inherently a read-modify-write operation and benefits from strict serialization. Consider batching or partial parallelism only if large uninstall sets become a common performance bottleneck.

Files changed (4) +1122 / -1

Enhancement (3) +401 / -1

admin.goAdd helper to delete per-repo WIF provider by repo name +12/-0

Add helper to delete per-repo WIF provider by repo name

• Introduces 'gcfProvisionerAdapter.deleteWIFProvider()' that validates 'owner/repo', builds a provider ID, and calls 'DeleteWIFProvider'. This enables inference-project WIF provider teardown in addition to mint deregistration.

internal/cli/admin.go

repos.goWire up 'repos remove' command and extend WIF deletion across projects +160/-1

Wire up 'repos remove' command and extend WIF deletion across projects

• Adds 'newReposRemoveCmd()' and 'runReposRemove()' with required '--repo' flags, '--dry-run', '--skip-wif-cleanup', and bounded '--concurrency'. Updates 'splitProjectAdapter.DeletePerRepoWIF' to deregister from mint first, then delete the WIF provider from the inference project when supported.

internal/cli/repos.go

remove.goImplement two-phase multi-repo uninstall orchestration +229/-0

Implement two-phase multi-repo uninstall orchestration

• Adds 'RemoveConfig', 'RemoveResult', and 'Remove()' to uninstall fullsend from explicit repos. Phase 1 performs bounded-parallel workflow deletion then variable/secret deletion (skipping vars/secrets on workflow failure); Phase 2 performs sequential WIF cleanup using manifest-resolved config when enabled.

internal/repos/remove.go

Tests (1) +721 / -0

remove_test.goAdd comprehensive unit tests for 'repos.Remove' behavior and failure modes +721/-0

Add comprehensive unit tests for 'repos.Remove' behavior and failure modes

• Adds a large test suite with fakes verifying dry-run behavior, installed/non-installed repos, workflow extension fallback, bounded concurrency, WIF sequential cleanup, partial failures, progress callbacks, and context cancellation semantics. Confirms WIF cleanup is skipped when appropriate (no manifest / repo not in manifest / phase-1 failure).

internal/repos/remove_test.go

[codecov]

Codecov Report

❌ Patch coverage is 77.62346% with 145 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/repos.go	65.96%	82 Missing and 31 partials ⚠️
internal/repos/manifest_edit.go	90.62%	6 Missing and 6 partials ⚠️
internal/repos/uninstall.go	90.76%	10 Missing and 2 partials ⚠️
internal/cli/admin.go	75.00%	1 Missing and 1 partial ⚠️
internal/forge/fake.go	0.00%	1 Missing and 1 partial ⚠️
internal/repos/batch_install.go	50.00%	1 Missing and 1 partial ⚠️
internal/repos/status.go	94.28%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 54 rules

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

1. Case-sensitive manifest lookup 🐞 Bug ≡ Correctness

Description

repos.Remove skips WIF cleanup when Manifest.ResolveConfig returns ok=false, but
ResolveConfig uses an exact (case-sensitive) owner/repo string match. If the user passes a
different casing than the manifest entry, the repo is treated as “not in manifest,” so mint
deregistration and WIF provider deletion are skipped while the repo can still be marked successful.

Code

internal/repos/remove.go[R128-134]

+			fullName := results[i].Owner + "/" + results[i].Repo
+			resolved, ok := cfg.Manifest.ResolveConfig(results[i].Owner, results[i].Repo)
+			if !ok {
+				progress(fullName, "wif", "Not in manifest, skipping WIF cleanup")
+				results[i].Success = true
+				continue
+			}

Relevance

⭐⭐⭐ High

PR fullsend-ai/fullsend#3692 shows team fixes casing bugs affecting WIF; likely accept case-insensitive manifest
ResolveConfig too.

PR-#3692
PR-#3002

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Remove treats ok=false from ResolveConfig as “repo not in manifest” and skips WIF cleanup.
ResolveConfig determines ok via a case-sensitive e.Repo == fullName comparison, so a casing
mismatch will incorrectly produce ok=false.

internal/repos/remove.go[128-134]
internal/repos/manifest.go[503-513]

Agent prompt

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

## Issue description
`repos.Remove` relies on `Manifest.ResolveConfig(...)->ok` to decide whether to run WIF cleanup. `ResolveConfig` performs a case-sensitive equality check against the manifest repo string, so a casing mismatch (CLI input vs manifest entry) incorrectly skips WIF cleanup.

## Issue Context
Repo names are effectively case-insensitive on GitHub, and users commonly copy/paste or type `--repo` values with different casing than the manifest file. Skipping WIF cleanup leaves PER_REPO_WIF_REPOS entries and the inference-project WIF provider behind, which defeats the intent of `repos remove`.

## Fix Focus Areas
- internal/repos/remove.go[128-134]
- internal/repos/manifest.go[503-513]

### Suggested approach
- Make `Manifest.ResolveConfig` perform a case-insensitive match (e.g., `strings.EqualFold(e.Repo, fullName)`), or normalize both manifest entries and lookup keys to a canonical form (typically lower-case) at load/validation time.
- Keep the returned `ResolvedConfig` values consistent (owner/repo fields can remain as provided or canonicalized), but ensure the correct manifest entry (including per-repo overrides) is found regardless of casing.

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

---

[fullsend-ai-review]

Looks good to me

Re-review after 7-commit update. Two of three prior low-severity findings are now resolved: AddToManifest duplicate detection uses strings.ToLower (fixing the case-sensitivity gap), and runReposAdd now validates --concurrency bounds when --install is set. The remaining low finding (splitOwnerRepo glob edge case) is unchanged and adequately mitigated by CLI-layer filtering. No new medium+ findings from the six-dimension review. The DeleteWIFProvider interface addition is cleanly implemented across all consumers — all WIFProvisioner implementors handle the new method. splitProjectAdapter.DeletePerRepoWIF correctly chains mint deregistration and inference WIF provider deletion with idempotent recovery guidance. The ProbeRepoState refactoring consolidates repo state probing into a single reusable function used by both repos init and repos status. Test coverage is thorough with 820 lines of uninstall tests, 512 lines of manifest edit tests, and comprehensive CLI-layer tests for all new commands. Documentation is complete across CLI docs, CLI internals command tree, operations guide, and the implementation plan.

Findings

Low

1. [logic-error] internal/repos/uninstall.go — splitOwnerRepo rejects glob characters via repoNamePattern, but MatchManifestRepos can return manifest entries that are themselves globs (e.g., acme/*). When the manifest literally contains acme/* as an entry and the user runs repos uninstall "acme/*", MatchManifestRepos returns ["acme/*"] via EqualFold match. Uninstall then calls splitOwnerRepo("acme/*") which fails with "invalid repo format". The same issue affects repos remove --uninstall. This is mitigated by the CLI-layer filtering in both runReposUninstall and runReposRemove, which skip glob entries via strings.ContainsAny(r, "*?[") before passing to Uninstall.

   Remediation: The CLI-layer mitigation is adequate. Consider adding a comment on splitOwnerRepo noting that callers must filter glob entries before calling Uninstall.

Previous run

Looks good to me

Re-review after rebase onto newer base. All prior low-severity findings from the previous review remain applicable on unchanged code — severity is anchored. No new medium+ findings emerged from the six-dimension review. The implementation is well-structured: three new CLI commands (repos add, repos remove, repos uninstall) plus the repos install positional-args migration align with the updated plan document (PR 8) and linked issue #4098. The DeleteWIFProvider interface addition is cleanly implemented across all consumers — verified that all WIFProvisioner implementors (6 in production code, 5 in test fakes) handle the new method. The splitProjectAdapter.DeletePerRepoWIF correctly calls both mint deregistration and inference WIF provider deletion with an idempotent error message ("re-run is safe"). The resolveConfigWithGlobs function properly falls back to glob-pattern matching when exact entry lookup fails, fixing the earlier concern about WIF cleanup for glob-matched repos. Test coverage is thorough with 820 lines of uninstall tests and 492 lines of manifest edit tests. Cross-repo contracts are not affected (WIFProvisioner is internal-only). No security issues found — input validation via repoNamePattern, confirmation prompts for glob patterns, proper dry-run enforcement, and safe error messages.

Findings

Low

1. [edge-case] internal/repos/manifest_edit.go — AddToManifest duplicate detection uses case-sensitive map lookup (existing[entry.Repo]), while matchesPattern (used by RemoveFromManifest, MatchManifestRepos, and filterRepos) uses strings.EqualFold. A manifest entry Acme/API would not be detected as a duplicate of acme/api, potentially creating duplicate entries. In practice GitHub normalizes repo names to lowercase, making this unlikely.

   Remediation: Normalize to lowercase when building the existing map: existing[strings.ToLower(e.Repo)] = true.

2. [logic-error] internal/repos/uninstall.go — splitOwnerRepo rejects glob characters via repoNamePattern, but MatchManifestRepos can return manifest entries that are themselves globs (e.g., acme/*). When the manifest literally contains acme/* as an entry and the user runs repos uninstall "acme/*", MatchManifestRepos returns ["acme/*"] via EqualFold match. Uninstall then calls splitOwnerRepo("acme/*") which fails with "invalid repo format". The same issue affects repos remove --uninstall. This is mitigated by the CLI-layer filtering in both runReposUninstall and runReposRemove, which skip glob entries via strings.ContainsAny(r, "*?[") before passing to Uninstall.

   Remediation: The CLI-layer mitigation is adequate. Consider adding a comment on splitOwnerRepo noting that callers must filter glob entries before passing to Uninstall.

3. [missing-validation] internal/cli/repos.go — runReposAdd does not validate --concurrency bounds (1-32). When --install is set, the value flows through to BatchInstall which validates it, but the error message arrives without repos add context. Both runReposUninstall and runReposRemove validate concurrency at their entry points.

   Remediation: Add if opts.install && (opts.concurrency < 1 || opts.concurrency > 32) validation at the top of runReposAdd.

---

Labels: PR adds repos management CLI subcommands and modifies install command

Previous run (2)

Looks good to me

Re-review after rebase onto newer base. The prior review's --roles docs finding is now resolved — the repos add flags table and CLI internals command tree both include --roles. All other prior findings from the first review remain applicable on unchanged code. The implementation is well-structured: three new CLI commands (repos add, repos remove, repos uninstall) plus the repos install positional-args migration align with the updated plan document and linked issue #4098. The DeleteWIFProvider interface addition is cleanly implemented across all consumers — verified that all WIFProvisioner implementors (6 in production code, 5 in test fakes) handle the new method. The splitProjectAdapter.DeletePerRepoWIF correctly calls both mint deregistration and inference WIF provider deletion with an idempotent error message ("re-run is safe"). Test coverage is thorough with 820 lines of uninstall tests and 492 lines of manifest edit tests. Cross-repo contracts are not affected (WIFProvisioner is internal-only). No security issues found — input validation via repoNamePattern, confirmation prompts for glob patterns, proper dry-run enforcement, and safe error messages.

Findings

Low

1. [logic-error] internal/repos/uninstall.go — splitOwnerRepo rejects glob characters via repoNamePattern, but MatchManifestRepos can return manifest entries that are themselves globs (e.g., acme/*). When the manifest literally contains acme/* as an entry and the user runs repos uninstall "acme/*", MatchManifestRepos returns ["acme/*"] via EqualFold match. Uninstall then calls splitOwnerRepo("acme/*") which fails with "invalid repo format". The same issue affects repos remove --uninstall. This is an edge case — manifests typically contain concrete repo names, and the error message is clear — but it could surprise operators who use glob manifest entries.

   Remediation: Filter or expand glob entries from MatchManifestRepos results before passing to Uninstall, or document that repos uninstall requires concrete repo names when the manifest contains glob entries.

2. [edge-case] internal/repos/manifest_edit.go — AddToManifest duplicate detection uses case-sensitive map lookup (existing[entry.Repo]), while matchesPattern (used by RemoveFromManifest, MatchManifestRepos, and filterRepos) uses strings.EqualFold. A manifest entry Acme/API would not be detected as a duplicate of acme/api, potentially creating duplicate entries. In practice GitHub normalizes repo names to lowercase, making this unlikely.

   Remediation: Normalize to lowercase when building the existing map: existing[strings.ToLower(e.Repo)] = true.

3. [missing-validation] internal/cli/repos.go — runReposAdd does not validate --concurrency bounds (1-32). When --install is set, the value flows through to BatchInstall which validates it, but the error message arrives without repos add context. Both runReposUninstall and runReposRemove validate concurrency at their entry points.

   Remediation: Add if opts.install && (opts.concurrency < 1 || opts.concurrency > 32) validation at the top of runReposAdd.

Previous run (3)

Looks good to me

Findings

Low

1. [logic-error] internal/repos/uninstall.go — splitOwnerRepo rejects glob characters via repoNamePattern, but MatchManifestRepos can return manifest entries that are themselves globs (e.g., acme/*). When the manifest literally contains acme/* as an entry and the user runs repos uninstall "acme/*", MatchManifestRepos returns ["acme/*"] via EqualFold match. Uninstall then calls splitOwnerRepo("acme/*") which fails with "invalid repo format". The same issue affects repos remove --uninstall. This is an edge case — manifests typically contain concrete repo names, and the error message is clear — but it could surprise operators who use glob manifest entries.

   Remediation: Filter or expand glob entries from MatchManifestRepos results before passing to Uninstall, or document that repos uninstall requires concrete repo names when the manifest contains glob entries.

2. [edge-case] internal/repos/manifest_edit.go — AddToManifest duplicate detection uses case-sensitive map lookup (existing[entry.Repo]), while matchesPattern (used by RemoveFromManifest, MatchManifestRepos, and filterRepos) uses strings.EqualFold. A manifest entry Acme/API would not be detected as a duplicate of acme/api, potentially creating duplicate entries. In practice GitHub normalizes repo names to lowercase, making this unlikely.

   Remediation: Normalize to lowercase when building the existing map: existing[strings.ToLower(e.Repo)] = true.

3. [missing-validation] internal/cli/repos.go — runReposAdd does not validate --concurrency bounds (1-32). When --install is set, the value flows through to BatchInstall which validates it, but the error message arrives without repos add context. Both runReposUninstall and runReposRemove validate concurrency at their entry points.

   Remediation: Add if opts.install && (opts.concurrency < 1 || opts.concurrency > 32) validation at the top of runReposAdd.

Previous run (4)

Looks good to me

Previous run (5)

Review — approve

Re-review after rebase. All prior review findings remain resolved. The implementation is well-structured: three new CLI commands (repos add, repos remove, repos uninstall) plus the repos install positional-args migration align with the updated plan document and linked issue #4098. The WIFProvisioner interface addition (DeleteWIFProvider) is cleanly implemented with idempotent underlying operations — both RemoveRepoFromMint (filters already-absent repo as no-op) and DeleteWIFProvider (returns nil on 404) are safe for re-runs, validating the error message's "re-run is safe" claim. Test coverage is thorough across all new commands, with confirmation prompt testing, concurrency validation, dry-run verification, and the splitProjectAdapter method routing test now verifying both mint deregistration and inference WIF provider deletion. Documentation is comprehensive across CLI docs, CLI internals tree, operations guide, and plan document. One low-severity follow-up remains from the prior review.

Findings

1. repos add docs omit --roles flag — low (docs-currency)

docs/cli/repos.md ~L185, docs/guides/dev/cli-internals.md ~L166 — The code registers a --roles flag on repos add (cmd.Flags().StringSliceVar(&opts.roles, "roles", config.PerRepoDefaultRoles(), "agent roles to install (used with --install)")), but neither the CLI docs flags table nor the CLI internals command tree include it. Users who want to specify non-default roles during repos add --install must discover the flag via --help. This was noted in the prior review.

Remediation: Add | --roles | triage,coder,review,fix,retro,prioritize | Agent roles to install (used with --install) | to the repos add flags table in docs/cli/repos.md, and add │ │ ├── --roles <list> to the repos add entry in docs/guides/dev/cli-internals.md.

---

Previous run (6)

Review — approve

All prior review findings are resolved. The filterRepos glob behavior now has four test cases (wildcard, question mark, no match, case-insensitive). Context cancellation during Phase 2 WIF cleanup now explicitly marks remaining repos with an error via WIF cleanup skipped: context canceled, backed by TestUninstall_ContextCancelledDuringPhase2_MarksRemaining. The splitProjectAdapter.DeletePerRepoWIF happy path is now tested via TestSplitProjectAdapter_MethodRouting which verifies both mint.DeletePerRepoWIF and inference.DeleteWIFProvider are called. Issue #4098 is now linked. The repos status --repo flag docs are updated to reflect glob support. One low-severity follow-up remains.

Findings

1. repos add --install lacks --roles flag — low (feature-gap)

internal/cli/repos.go ~L572 — runReposAdd with --install constructs a reposInstallConfig without setting roles. The BatchInstall fallback at batch_install.go:357-358 applies config.PerRepoDefaultRoles() when roles is empty, so the default roles are correctly installed. However, unlike repos install which exposes --roles for custom role selection, repos add --install has no way to specify non-default roles. Users needing custom roles must run repos add followed by repos install --roles ... separately.

Remediation: Add a --roles flag to repos add (used only with --install), or document that repos add --install always uses default roles.

Previous run (7)

Review — approve

All prior review findings are resolved. The type assertion issue in splitProjectAdapter.DeletePerRepoWIF was fixed by adding DeleteWIFProvider to the WIFProvisioner interface, the non-atomic error message now includes recovery guidance ("re-run is safe"), splitOwnerRepo now uses repoNamePattern, plan document and docs are fully updated, and the commit message carries the required BREAKING CHANGE: trailer. The remaining items below are low-severity follow-ups.

Findings

1. filterRepos glob behavior untested — low (test-adequacy)

internal/repos/status_test.go ~L625 — TestFilterRepos has four subtests but all exercise exact-match and case-insensitive paths. The filterRepos function was changed from map-based exact matching to matchesPattern (glob-aware via filepath.Match), but no test verifies the glob code path. The glob behavior is now used by both repos install (positional args) and repos status --repo, so a gap here leaves the new feature path unexercised.

Remediation: Add test cases like filterRepos(repos, []string{"acme-corp/*"}) to verify glob matching and edge cases (e.g., patterns that match nothing, patterns with ?).

2. Context cancellation reports success for WIF-incomplete repos — low (error-handling)

internal/repos/uninstall.go ~L156 — When context is cancelled during Phase 2 (sequential WIF cleanup), the loop breaks but repos that completed Phase 1 (workflow/vars/secrets deleted) but had WIF cleanup skipped get Success=true because Error is nil. These repos have their workflow and secrets deleted but WIF provider and mint registration remain, creating an inconsistent state that is silently reported as successful.

Remediation: Either set Error for remaining Phase-1-successful repos when context cancellation breaks the Phase 2 loop, or add a WIFSkipped field to UninstallResult.

3. repos add --install lacks --roles flag — low (feature-gap)

internal/cli/repos.go ~L572 — runReposAdd with --install constructs a reposInstallConfig without setting roles, so it always uses default roles. Unlike repos install which exposes --roles, repos add --install has no way to specify custom roles. This is a minor feature gap.

Remediation: Add a --roles flag to repos add (used only with --install), or document that repos add --install always uses default roles.

4. No linked GitHub issue — low (traceability)

No linked issue for a 3000+ line change with breaking changes. The PR references ADR-0057 PR 8, updates the implementation plan, and the commit message documents the scope clearly, which provides adequate traceability. A tracking issue would be standard practice for changes of this size.

Previous run (8)

Review — approve

All prior review findings are resolved. The filterRepos glob behavior now has four test cases (wildcard, question mark, no match, case-insensitive). Context cancellation during Phase 2 WIF cleanup now explicitly marks remaining repos with an error via WIF cleanup skipped: context canceled, backed by TestUninstall_ContextCancelledDuringPhase2_MarksRemaining. The splitProjectAdapter.DeletePerRepoWIF happy path is now tested via TestSplitProjectAdapter_MethodRouting which verifies both mint.DeletePerRepoWIF and inference.DeleteWIFProvider are called. Issue #4098 is now linked. The repos status --repo flag docs are updated to reflect glob support. One low-severity follow-up remains.

Findings

1. repos add --install lacks --roles flag — low (feature-gap)

internal/cli/repos.go ~L572 — runReposAdd with --install constructs a reposInstallConfig without setting roles. The BatchInstall fallback at batch_install.go:357-358 applies config.PerRepoDefaultRoles() when roles is empty, so the default roles are correctly installed. However, unlike repos install which exposes --roles for custom role selection, repos add --install has no way to specify non-default roles. Users needing custom roles must run repos add followed by repos install --roles ... separately.

Remediation: Add a --roles flag to repos add (used only with --install), or document that repos add --install always uses default roles.

---

Previous run (9)

Review — approve

All prior review findings are resolved. The type assertion issue in splitProjectAdapter.DeletePerRepoWIF was fixed by adding DeleteWIFProvider to the WIFProvisioner interface, the non-atomic error message now includes recovery guidance ("re-run is safe"), splitOwnerRepo now uses repoNamePattern, plan document and docs are fully updated, and the commit message carries the required BREAKING CHANGE: trailer. The remaining items below are low-severity follow-ups.

Findings

1. filterRepos glob behavior untested — low (test-adequacy)

internal/repos/status_test.go ~L625 — TestFilterRepos has four subtests but all exercise exact-match and case-insensitive paths. The filterRepos function was changed from map-based exact matching to matchesPattern (glob-aware via filepath.Match), but no test verifies the glob code path. The glob behavior is now used by both repos install (positional args) and repos status --repo, so a gap here leaves the new feature path unexercised.

Remediation: Add test cases like filterRepos(repos, []string{"acme-corp/*"}) to verify glob matching and edge cases (e.g., patterns that match nothing, patterns with ?).

2. Context cancellation reports success for WIF-incomplete repos — low (error-handling)

internal/repos/uninstall.go ~L156 — When context is cancelled during Phase 2 (sequential WIF cleanup), the loop breaks but repos that completed Phase 1 (workflow/vars/secrets deleted) but had WIF cleanup skipped get Success=true because Error is nil. These repos have their workflow and secrets deleted but WIF provider and mint registration remain, creating an inconsistent state that is silently reported as successful.

Remediation: Either set Error for remaining Phase-1-successful repos when context cancellation breaks the Phase 2 loop, or add a WIFSkipped field to UninstallResult.

3. repos add --install lacks --roles flag — low (feature-gap)

internal/cli/repos.go ~L572 — runReposAdd with --install constructs a reposInstallConfig without setting roles, so it always uses default roles. Unlike repos install which exposes --roles, repos add --install has no way to specify custom roles. This is a minor feature gap.

Remediation: Add a --roles flag to repos add (used only with --install), or document that repos add --install always uses default roles.

4. No linked GitHub issue — low (traceability)

No linked issue for a 3000+ line change with breaking changes. The PR references ADR-0057 PR 8, updates the implementation plan, and the commit message documents the scope clearly, which provides adequate traceability. A tracking issue would be standard practice for changes of this size.

Previous run (10)

Review — approve

All prior review findings are resolved. The type assertion issue in splitProjectAdapter.DeletePerRepoWIF was fixed by adding DeleteWIFProvider to the WIFProvisioner interface, the non-atomic error message now includes recovery guidance ("re-run is safe"), splitOwnerRepo now uses repoNamePattern, plan document and docs are fully updated, and the commit message carries the required BREAKING CHANGE: trailer. The remaining items below are low-severity follow-ups.

Findings

1. filterRepos glob behavior untested — low (test-adequacy)

internal/repos/status_test.go ~L625 — TestFilterRepos has four subtests but all exercise exact-match and case-insensitive paths. The filterRepos function was changed from map-based exact matching to matchesPattern (glob-aware via filepath.Match), but no test verifies the glob code path. The glob behavior is now used by both repos install (positional args) and repos status --repo, so a gap here leaves the new feature path unexercised.

Remediation: Add test cases like filterRepos(repos, []string{"acme-corp/*"}) to verify glob matching and edge cases (e.g., patterns that match nothing, patterns with ?).

2. Context cancellation reports success for WIF-incomplete repos — low (error-handling)

internal/repos/uninstall.go ~L156 — When context is cancelled during Phase 2 (sequential WIF cleanup), the loop breaks but repos that completed Phase 1 (workflow/vars/secrets deleted) but had WIF cleanup skipped get Success=true because Error is nil. These repos have their workflow and secrets deleted but WIF provider and mint registration remain, creating an inconsistent state that is silently reported as successful.

Remediation: Either set Error for remaining Phase-1-successful repos when context cancellation breaks the Phase 2 loop, or add a WIFSkipped field to UninstallResult.

3. repos add --install lacks --roles flag — low (feature-gap)

internal/cli/repos.go ~L572 — runReposAdd with --install constructs a reposInstallConfig without setting roles, so it always uses default roles. Unlike repos install which exposes --roles, repos add --install has no way to specify custom roles. This is a minor feature gap.

Remediation: Add a --roles flag to repos add (used only with --install), or document that repos add --install always uses default roles.

4. No linked GitHub issue — low (traceability)

No linked issue for a 3000+ line change with breaking changes. The PR references ADR-0057 PR 8, updates the implementation plan, and the commit message documents the scope clearly, which provides adequate traceability. A tracking issue would be standard practice for changes of this size.

---

Previous run (11)

Review — comment

Substantial improvement since the prior review. All prior high-severity findings (scope-creep, missing documentation) are resolved — the plan document now reflects the three-command split, full user-facing docs are added for repos add, repos remove, and repos uninstall, and the PR title correctly carries the ! suffix for the breaking --repo removal. Prior medium findings (glob WIF cleanup, silent type-assertion failure, CLI internals staleness, operations guide) are also addressed. Three medium findings remain.

Findings

1. repos status --repo flag: docs removed but code retained — medium (docs-currency)

internal/cli/repos.go ~L189 / docs/cli/repos.md — The PR removes --repo from the repos status flags table and examples in the docs, but the code still registers the flag (cmd.Flags().StringArrayVar(&repoFilter, "repo", ...) in newReposStatusCmd). Additionally, filterRepos in status.go was updated from exact-match to matchesPattern (glob-aware), silently changing the flag's semantics. The flag now supports glob patterns — but this is neither documented nor removed.

Remediation: Either remove the --repo flag from the status command code (matching the docs), or restore the documentation with updated semantics showing glob support. If removing, consider that this is an additional breaking change.

2. No test coverage for splitProjectAdapter.DeletePerRepoWIF production path — medium (test-adequacy)

internal/cli/repos_test.go ~L858 — The test TestSplitProjectAdapter_MethodRouting was changed from asserting success (require.NoError) to asserting failure (require.Error), which correctly validates the new error-on-type-assertion-failure behavior. However, no test now exercises the happy path — the production path where s.inference is a *gcfProvisionerAdapter and both mint deregistration and WIF provider deletion succeed. The uninstall integration tests (TestRunReposUninstall_Success, TestRunReposRemove_WithUninstall) use buildProvisionerFactory with a non-nil testProv, so the factory returns testProv directly, never constructing a splitProjectAdapter. The sequential mint-deregister → delete-provider flow through splitProjectAdapter.DeletePerRepoWIF has zero test coverage.

Remediation: Add a test that constructs a splitProjectAdapter with both mint and inference set to *gcfProvisionerAdapter instances (or a test double that satisfies the type assertion) and verifies that DeletePerRepoWIF calls both DeletePerRepoWIF on mint and deleteWIFProvider on inference in sequence.

3. Non-atomic WIF cleanup leaves inconsistent state on partial failure — medium (error-handling)

internal/cli/repos.go ~L940 — splitProjectAdapter.DeletePerRepoWIF calls s.mint.DeletePerRepoWIF (deregistering from mint) before inf.deleteWIFProvider (deleting WIF provider). If the second call fails, the repo is deregistered from the mint but the WIF provider still exists in GCP. The error is propagated, so the caller sees a failure, but the error message ("deleting WIF provider: ...") doesn't indicate that the mint deregistration already succeeded. Operators recovering from this failure won't know whether re-running is safe (it would attempt to deregister from mint again for a repo that's already deregistered).

Remediation: Wrap the error with context indicating partial completion: fmt.Errorf("deleting WIF provider (mint deregistration already succeeded for %s): %w", repo, err). Consider whether DeletePerRepoWIF on mint is idempotent — if so, document this so operators know re-running is safe.

4. Plan document stale --repo references — low (docs-currency)

docs/plans/repos-management.md lines 848, 1175 — The detailed implementation spec sections still reference --repo for repos install (line 848: --repo (string, repeatable): install specific repos only) and the old repos remove design (line 1175: --repo (repeatable, required): repos to remove). Both are stale: install now uses positional args, and remove uses positional args with glob support.

Remediation: Update the detailed spec sections to match the implemented design.

5. splitOwnerRepo lacks strict validation — low (input-validation)

internal/repos/uninstall.go ~L218 — Validates only that input contains / with non-empty parts. Does not apply repoNamePattern regex (^[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+$) used in AddToManifest. Downstream API calls would reject invalid names, but the inconsistency creates an unvalidated input path.

Remediation: Apply the same repoNamePattern validation, or extract a shared validation helper.

6. No linked GitHub issue — low (traceability)

No linked issue for a 2700+ line change with breaking changes. The PR references ADR-0057 PR 8 and updates the plan document, which provides traceability to the approved design. Standard practice for non-trivial changes is a tracking issue.

Previous run (12)

Review — comment

Substantial improvement since the prior review. All prior high-severity findings (scope-creep, missing documentation) are resolved — the plan document now reflects the three-command split, full user-facing docs are added for repos add, repos remove, and repos uninstall, and the PR title correctly carries the ! suffix for the breaking --repo removal. Prior medium findings (glob WIF cleanup, silent type-assertion failure, CLI internals staleness, operations guide) are also addressed. Three medium findings remain.

Findings

1. repos status --repo flag: docs removed but code retained — medium (docs-currency)

internal/cli/repos.go ~L189 / docs/cli/repos.md — The PR removes --repo from the repos status flags table and examples in the docs, but the code still registers the flag (cmd.Flags().StringArrayVar(&repoFilter, "repo", ...) in newReposStatusCmd). Additionally, filterRepos in status.go was updated from exact-match to matchesPattern (glob-aware), silently changing the flag's semantics. The flag now supports glob patterns — but this is neither documented nor removed.

Remediation: Either remove the --repo flag from the status command code (matching the docs), or restore the documentation with updated semantics showing glob support. If removing, consider that this is an additional breaking change.

2. No test coverage for splitProjectAdapter.DeletePerRepoWIF production path — medium (test-adequacy)

internal/cli/repos_test.go ~L858 — The test TestSplitProjectAdapter_MethodRouting was changed from asserting success (require.NoError) to asserting failure (require.Error), which correctly validates the new error-on-type-assertion-failure behavior. However, no test now exercises the happy path — the production path where s.inference is a *gcfProvisionerAdapter and both mint deregistration and WIF provider deletion succeed. The uninstall integration tests (TestRunReposUninstall_Success, TestRunReposRemove_WithUninstall) use buildProvisionerFactory with a non-nil testProv, so the factory returns testProv directly, never constructing a splitProjectAdapter. The sequential mint-deregister → delete-provider flow through splitProjectAdapter.DeletePerRepoWIF has zero test coverage.

Remediation: Add a test that constructs a splitProjectAdapter with both mint and inference set to *gcfProvisionerAdapter instances (or a test double that satisfies the type assertion) and verifies that DeletePerRepoWIF calls both DeletePerRepoWIF on mint and deleteWIFProvider on inference in sequence.

3. Non-atomic WIF cleanup leaves inconsistent state on partial failure — medium (error-handling)

internal/cli/repos.go ~L940 — splitProjectAdapter.DeletePerRepoWIF calls s.mint.DeletePerRepoWIF (deregistering from mint) before inf.deleteWIFProvider (deleting WIF provider). If the second call fails, the repo is deregistered from the mint but the WIF provider still exists in GCP. The error is propagated, so the caller sees a failure, but the error message ("deleting WIF provider: ...") doesn't indicate that the mint deregistration already succeeded. Operators recovering from this failure won't know whether re-running is safe (it would attempt to deregister from mint again for a repo that's already deregistered).

Remediation: Wrap the error with context indicating partial completion: fmt.Errorf("deleting WIF provider (mint deregistration already succeeded for %s): %w", repo, err). Consider whether DeletePerRepoWIF on mint is idempotent — if so, document this so operators know re-running is safe.

4. Plan document stale --repo references — low (docs-currency)

docs/plans/repos-management.md lines 848, 1175 — The detailed implementation spec sections still reference --repo for repos install (line 848: --repo (string, repeatable): install specific repos only) and the old repos remove design (line 1175: --repo (repeatable, required): repos to remove). Both are stale: install now uses positional args, and remove uses positional args with glob support.

Remediation: Update the detailed spec sections to match the implemented design.

5. splitOwnerRepo lacks strict validation — low (input-validation)

internal/repos/uninstall.go ~L218 — Validates only that input contains / with non-empty parts. Does not apply repoNamePattern regex (^[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+$) used in AddToManifest. Downstream API calls would reject invalid names, but the inconsistency creates an unvalidated input path.

Remediation: Apply the same repoNamePattern validation, or extract a shared validation helper.

6. No linked GitHub issue — low (traceability)

No linked issue for a 2700+ line change with breaking changes. The PR references ADR-0057 PR 8 and updates the plan document, which provides traceability to the approved design. Standard practice for non-trivial changes is a tracking issue.

---

Previous run (13)

Review — comment

Findings

High

1. [scope-creep] internal/cli/repos.go — PR implements three commands (repos add, repos remove, repos uninstall) that exceed the ADR-0057 PR 8 specification, which defines a single repos remove command for infrastructure teardown and explicitly states "Does NOT remove repos from the manifest." The implementation inverts the plan's semantics: repos remove edits the manifest while repos uninstall handles infrastructure teardown. The repos add command is not in the PR 8 spec at all.
   Remediation: Update docs/plans/repos-management.md PR 8 section to reflect the three-command split and revised semantics, or align command naming with the plan.

2. [missing-doc] docs/cli/repos.md:14 — The commands table lists only repos init, repos install, and repos status. Three new CLI commands (repos add, repos remove, repos uninstall) have zero documentation — no command entries, no flag descriptions, no usage examples.
   Remediation: Add entries for all three new commands to the commands table and create full documentation sections for each, including flags, behavior description, and examples.

Medium

3. [logic-error] internal/repos/uninstall.go:167 — WIF cleanup is silently skipped for repos matched via manifest glob entries. Uninstall Phase 2 calls cfg.Manifest.ResolveConfig(owner, repo) which only does exact string matching on the Repo field. If the manifest contains a glob entry like acme/*, ResolveConfig("acme", "api") returns ok=false and WIF cleanup is skipped with a misleading "Not in manifest" message, causing WIF provider leaks. The ResolveConfig comment explicitly says: "For repos matched via glob expansion, use ResolveConfigForEntry instead."
   Remediation: Use ResolveConfigForEntry with the matching entry, or call ExpandGlobs first to resolve glob entries before looking up config.

4. [silent-failure] internal/cli/repos.go:930 — In splitProjectAdapter.DeletePerRepoWIF, the WIF provider deletion from the inference project is gated on a type assertion (if inf, ok := s.inference.(*gcfProvisionerAdapter); ok). When the assertion fails (which occurs in all tests since test provisioners use *trackingProvisioner), the deletion is silently skipped with no log or error. This means: (a) the dual-deletion behavior has zero test coverage through this code path, and (b) in production, if s.inference is ever not a *gcfProvisionerAdapter, the WIF provider in the inference project leaks silently.
   Remediation: Add a DeleteWIFProvider method to the WIFProvisioner interface (avoiding the type assertion), or add an else branch that logs a warning. Update tests to verify both mint deregistration and inference WIF provider deletion.

5. [stale-doc] docs/cli/repos.md (lines 76, 86, 109) — Multiple stale references to the removed --repo flag on repos install. Line 76: example fullsend repos install --repo acme/api --repo acme/web. Line 86: flags table entry for --repo. Line 109: another example using --repo.
   Remediation: Update all examples and the flags table to use positional arguments.

6. [stale-doc] docs/guides/dev/cli-internals.md:54 — CLI command tree shows --repo <owner/repo> under repos install with comment "Install specific repos only (repeatable)". This flag has been removed.
   Remediation: Update the command tree to show positional args and add entries for the three new commands.

7. [breaking-cli] internal/cli/repos.go:323 — The --repo flag is removed from repos install and replaced with positional arguments. Per COMMITS.md, removing a flag is a breaking change that requires the ! suffix in the PR title (feat(repos)!: ...) and a BREAKING CHANGE: trailer in the commit message. The current PR title lacks both markers, making the breaking change invisible in release notes.
   Remediation: Update PR title to feat(repos)!: add repos remove command for uninstalling fullsend and add a BREAKING CHANGE: trailer explaining that repos install --repo is replaced by positional arguments.

Low

8. [behavioral-change] internal/cli/repos.go:927 — splitProjectAdapter.DeletePerRepoWIF now also deletes the WIF provider from the inference project, which also affects batch_install.go cleanup paths (lines ~302, ~384). This is actually correct behavior (fully undo provisioning on failure), but it is a non-obvious side-effect beyond the repos remove scope.

9. [missing-authorization] — No linked GitHub issue establishes authorization for this 2300+ line change. The PR references ADR-0057 PR 8 which provides traceability, but a linked issue is the standard for non-trivial changes.

10. [naming-divergence] internal/repos/uninstall.go — UninstallResult uses a single WIFDeregistered field, diverging from the plan spec which defines separate WIFDeregistered and WIFDeleted fields to track mint deregistration and WIF provider deletion independently.

11. [missing-concurrency-validation] internal/cli/repos.go — runReposRemove does not validate --concurrency before loading the manifest, unlike runReposUninstall which validates immediately. Invalid concurrency values waste work before eventually failing in repos.Uninstall.

12. [input-validation] internal/repos/uninstall.go:218 — splitOwnerRepo validates only that input contains / with non-empty parts. Does not apply githubOwnerPattern/githubRepoPattern validation used in other CLI entry points (admin.go, github.go, inference.go).

13. [input-validation] internal/cli/repos.go — runReposUninstall, runReposRemove, and runReposAdd load manifests via LoadManifest but never call manifest.Validate(), unlike runReposStatus and BatchInstall which validate after loading.

14. [input-validation] internal/repos/manifest_edit.go:56 — AddToManifest accepts repo names without format validation, allowing arbitrary strings to be persisted into the manifest file.

15. [missing-doc] docs/guides/dev/cli-internals.md:59 — CLI command tree under repos is missing entries for add, remove, and uninstall.

16. [missing-doc] docs/guides/getting-started/operations.md:74 — Operations table lists repos init, repos install, and repos status but is missing the three new commands.

Previous run (14)

Review

Findings

High

1. [scope-creep] internal/cli/repos.go — PR implements three commands (repos add, repos remove, repos uninstall) that exceed the ADR-0057 PR 8 specification, which defines a single repos remove command for infrastructure teardown and explicitly states "Does NOT remove repos from the manifest." The implementation inverts the plan's semantics: repos remove edits the manifest while repos uninstall handles infrastructure teardown. The repos add command is not in the PR 8 spec at all.
   Remediation: Update docs/plans/repos-management.md PR 8 section to reflect the three-command split and revised semantics, or align command naming with the plan.

2. [missing-doc] docs/cli/repos.md:14 — The commands table lists only repos init, repos install, and repos status. Three new CLI commands (repos add, repos remove, repos uninstall) have zero documentation — no command entries, no flag descriptions, no usage examples.
   Remediation: Add entries for all three new commands to the commands table and create full documentation sections for each, including flags, behavior description, and examples.

Medium

3. [logic-error] internal/repos/uninstall.go:167 — WIF cleanup is silently skipped for repos matched via manifest glob entries. Uninstall Phase 2 calls cfg.Manifest.ResolveConfig(owner, repo) which only does exact string matching on the Repo field. If the manifest contains a glob entry like acme/*, ResolveConfig("acme", "api") returns ok=false and WIF cleanup is skipped with a misleading "Not in manifest" message, causing WIF provider leaks. The ResolveConfig comment explicitly says: "For repos matched via glob expansion, use ResolveConfigForEntry instead."
   Remediation: Use ResolveConfigForEntry with the matching entry, or call ExpandGlobs first to resolve glob entries before looking up config.

4. [silent-failure] internal/cli/repos.go:930 — In splitProjectAdapter.DeletePerRepoWIF, the WIF provider deletion from the inference project is gated on a type assertion (if inf, ok := s.inference.(*gcfProvisionerAdapter); ok). When the assertion fails (which occurs in all tests since test provisioners use *trackingProvisioner), the deletion is silently skipped with no log or error. This means: (a) the dual-deletion behavior has zero test coverage through this code path, and (b) in production, if s.inference is ever not a *gcfProvisionerAdapter, the WIF provider in the inference project leaks silently.
   Remediation: Add a DeleteWIFProvider method to the WIFProvisioner interface (avoiding the type assertion), or add an else branch that logs a warning. Update tests to verify both mint deregistration and inference WIF provider deletion.

5. [stale-doc] docs/cli/repos.md (lines 76, 86, 109) — Multiple stale references to the removed --repo flag on repos install. Line 76: example fullsend repos install --repo acme/api --repo acme/web. Line 86: flags table entry for --repo. Line 109: another example using --repo.
   Remediation: Update all examples and the flags table to use positional arguments.

6. [stale-doc] docs/guides/dev/cli-internals.md:54 — CLI command tree shows --repo <owner/repo> under repos install with comment "Install specific repos only (repeatable)". This flag has been removed.
   Remediation: Update the command tree to show positional args and add entries for the three new commands.

7. [breaking-cli] internal/cli/repos.go:323 — The --repo flag is removed from repos install and replaced with positional arguments. Per COMMITS.md, removing a flag is a breaking change that requires the ! suffix in the PR title (feat(repos)!: ...) and a BREAKING CHANGE: trailer in the commit message. The current PR title lacks both markers, making the breaking change invisible in release notes.
   Remediation: Update PR title to feat(repos)!: add repos remove command for uninstalling fullsend and add a BREAKING CHANGE: trailer explaining that repos install --repo is replaced by positional arguments.

Low

8. [behavioral-change] internal/cli/repos.go:927 — splitProjectAdapter.DeletePerRepoWIF now also deletes the WIF provider from the inference project, which also affects batch_install.go cleanup paths (lines ~302, ~384). This is actually correct behavior (fully undo provisioning on failure), but it is a non-obvious side-effect beyond the repos remove scope.

9. [missing-authorization] — No linked GitHub issue establishes authorization for this 2300+ line change. The PR references ADR-0057 PR 8 which provides traceability, but a linked issue is the standard for non-trivial changes.

10. [naming-divergence] internal/repos/uninstall.go — UninstallResult uses a single WIFDeregistered field, diverging from the plan spec which defines separate WIFDeregistered and WIFDeleted fields to track mint deregistration and WIF provider deletion independently.

11. [missing-concurrency-validation] internal/cli/repos.go — runReposRemove does not validate --concurrency before loading the manifest, unlike runReposUninstall which validates immediately. Invalid concurrency values waste work before eventually failing in repos.Uninstall.

12. [input-validation] internal/repos/uninstall.go:218 — splitOwnerRepo validates only that input contains / with non-empty parts. Does not apply githubOwnerPattern/githubRepoPattern validation used in other CLI entry points (admin.go, github.go, inference.go).

13. [input-validation] internal/cli/repos.go — runReposUninstall, runReposRemove, and runReposAdd load manifests via LoadManifest but never call manifest.Validate(), unlike runReposStatus and BatchInstall which validate after loading.

14. [input-validation] internal/repos/manifest_edit.go:56 — AddToManifest accepts repo names without format validation, allowing arbitrary strings to be persisted into the manifest file.

15. [missing-doc] docs/guides/dev/cli-internals.md:59 — CLI command tree under repos is missing entries for add, remove, and uninstall.

16. [missing-doc] docs/guides/getting-started/operations.md:74 — Operations table lists repos init, repos install, and repos status but is missing the three new commands.

---

Previous run (15)

Review — comment

Well-structured implementation of repos remove (PR 8 from the repos-management plan). The two-phase parallel/sequential pattern mirrors repos install appropriately, tests are thorough (25 tests, good edge case coverage), and the code follows established conventions. Four medium-severity findings below — none blocking, but all worth addressing before or shortly after merge.

Findings

1. Silent WIF provider leak on type assertion failure — medium (correctness)

internal/cli/repos.go ~L655 — The modified splitProjectAdapter.DeletePerRepoWIF uses a type assertion (if inf, ok := s.inference.(*gcfProvisionerAdapter); ok) to call the new deleteWIFProvider method. If the assertion fails, WIF provider deletion from the inference project is silently skipped — no log, no error, no signal. In production today, inference is always *gcfProvisionerAdapter, so this is safe. But a future refactor changing the concrete type would silently leak WIF providers in GCP with no failure signal.

Remediation: Either add DeleteWIFProvider to the WIFProvisioner interface (avoiding the type assertion entirely), or add an else branch that logs a warning / returns an error so the skip is observable.

2. No confirmation prompt for destructive operations — medium (safety)

internal/cli/repos.go newReposRemoveCmd — The command performs irreversible destructive operations (deleting workflow files, secrets, variables, and WIF providers) without any interactive confirmation. Compare with the existing admin uninstall command which requires typing the organization name to confirm. While --dry-run exists, a typo in a --repo flag during a live run could silently destroy infrastructure in the wrong repo.

Remediation: Add a confirmation prompt (or --yes/--force flag to bypass) in runReposRemove before calling repos.Remove when DryRun is false. List affected repos and require explicit acknowledgment, consistent with admin uninstall.

3. --manifest-only flag exceeds plan spec — medium (scope)

internal/repos/remove.go — The implementation plan (repos-management.md PR 8 section, line 1253) explicitly states: "Does NOT remove repos from the manifest — operator edits repos.yaml manually." The PR adds --manifest-only which does exactly the opposite. While this is a useful convenience, it is unspecified scope. The field naming also diverges: the plan specifies WIFDeleted but the implementation uses WIFDeregistered.

Remediation: Update the plan document to reflect the --manifest-only addition and the naming choice. No code change needed — the feature makes sense.

4. New CLI command not documented — medium (docs-currency)

docs/cli/repos.md — The commands table lists only repos init, repos install, and repos status. No entry or section for repos remove. The operations guide (docs/guides/getting-started/operations.md) describes manual per-repo teardown steps but doesn't reference the new automated command.

Remediation: Add a repos remove row to the commands table and a full ## repos remove section documenting flags, two-phase behavior, and the --manifest-only mode. Add a cross-reference in the operations guide's "Per-repo teardown" section.

5. Implicit behavioral change to install cleanup — low (correctness)

internal/cli/repos.go ~L655 — splitProjectAdapter.DeletePerRepoWIF is also called from batch_install.go cleanup paths (lines ~302 and ~384) when WIF registration or scaffold commit fails during repos install. With this PR, those cleanup paths now additionally delete the WIF provider from the inference project. This is actually correct behavior (fully undo provisioning on failure — previously the WIF provider was leaked on failed installs), but it is a non-obvious side-effect beyond the repos remove feature scope.

Remediation: Mention this behavioral improvement in the PR description.

6. Input validation gap on repo names — low (security)

internal/repos/remove.go splitOwnerRepo — Validates only that input contains / with non-empty parts. Does not apply githubOwnerPattern/githubRepoPattern validation used in other CLI entry points (e.g., admin.go runPerRepoInstall). Downstream GitHub API calls would reject invalid names, but the inconsistency creates an unvalidated input path.

Remediation: Apply the same regex validation as other CLI commands, or extract a shared validateOwnerRepo helper.

Previous run (16)

Review — comment

Well-structured implementation of repos remove (PR 8 from the repos-management plan). The two-phase parallel/sequential pattern mirrors repos install appropriately, tests are thorough (25 tests, good edge case coverage), and the code follows established conventions. Four medium-severity findings below — none blocking, but all worth addressing before or shortly after merge.

Findings

1. Silent WIF provider leak on type assertion failure — medium (correctness)

internal/cli/repos.go ~L655 — The modified splitProjectAdapter.DeletePerRepoWIF uses a type assertion (if inf, ok := s.inference.(*gcfProvisionerAdapter); ok) to call the new deleteWIFProvider method. If the assertion fails, WIF provider deletion from the inference project is silently skipped — no log, no error, no signal. In production today, inference is always *gcfProvisionerAdapter, so this is safe. But a future refactor changing the concrete type would silently leak WIF providers in GCP with no failure signal.

Remediation: Either add DeleteWIFProvider to the WIFProvisioner interface (avoiding the type assertion entirely), or add an else branch that logs a warning / returns an error so the skip is observable.

2. No confirmation prompt for destructive operations — medium (safety)

internal/cli/repos.go newReposRemoveCmd — The command performs irreversible destructive operations (deleting workflow files, secrets, variables, and WIF providers) without any interactive confirmation. Compare with the existing admin uninstall command which requires typing the organization name to confirm. While --dry-run exists, a typo in a --repo flag during a live run could silently destroy infrastructure in the wrong repo.

Remediation: Add a confirmation prompt (or --yes/--force flag to bypass) in runReposRemove before calling repos.Remove when DryRun is false. List affected repos and require explicit acknowledgment, consistent with admin uninstall.

3. --manifest-only flag exceeds plan spec — medium (scope)

internal/repos/remove.go — The implementation plan (repos-management.md PR 8 section, line 1253) explicitly states: "Does NOT remove repos from the manifest — operator edits repos.yaml manually." The PR adds --manifest-only which does exactly the opposite. While this is a useful convenience, it is unspecified scope. The field naming also diverges: the plan specifies WIFDeleted but the implementation uses WIFDeregistered.

Remediation: Update the plan document to reflect the --manifest-only addition and the naming choice. No code change needed — the feature makes sense.

4. New CLI command not documented — medium (docs-currency)

docs/cli/repos.md — The commands table lists only repos init, repos install, and repos status. No entry or section for repos remove. The operations guide (docs/guides/getting-started/operations.md) describes manual per-repo teardown steps but doesn't reference the new automated command.

Remediation: Add a repos remove row to the commands table and a full ## repos remove section documenting flags, two-phase behavior, and the --manifest-only mode. Add a cross-reference in the operations guide's "Per-repo teardown" section.

5. Implicit behavioral change to install cleanup — low (correctness)

internal/cli/repos.go ~L655 — splitProjectAdapter.DeletePerRepoWIF is also called from batch_install.go cleanup paths (lines ~302 and ~384) when WIF registration or scaffold commit fails during repos install. With this PR, those cleanup paths now additionally delete the WIF provider from the inference project. This is actually correct behavior (fully undo provisioning on failure — previously the WIF provider was leaked on failed installs), but it is a non-obvious side-effect beyond the repos remove feature scope.

Remediation: Mention this behavioral improvement in the PR description.

6. Input validation gap on repo names — low (security)

internal/repos/remove.go splitOwnerRepo — Validates only that input contains / with non-empty parts. Does not apply githubOwnerPattern/githubRepoPattern validation used in other CLI entry points (e.g., admin.go runPerRepoInstall). Downstream GitHub API calls would reject invalid names, but the inconsistency creates an unvalidated input path.

Remediation: Apply the same regex validation as other CLI commands, or extract a shared validateOwnerRepo helper.

---

Labels: PR adds new repos CLI subcommand and modifies install infrastructure

Previous run (17)

Review

Reason: stale-head

The review agent reviewed commit 9e2b73d6d1a32976dbae0817a9b616227b4ea661 but the PR HEAD is now 34d2a82254a29698474c816f8265463c92ac22f5. This review was discarded to avoid approving unreviewed code.

[fullsend-ai-review]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:04 PM UTC · Completed 10:16 PM UTC
Commit: 34d2a82 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:56 PM UTC · Ended 11:01 PM UTC
Commit: 2941769 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 11:01 PM UTC · Completed 11:18 PM UTC
Commit: 0194795 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 2:04 AM UTC · Completed 2:17 AM UTC
Commit: eb082a4 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 2:58 AM UTC · Ended 3:07 AM UTC
Commit: 2941769 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:08 AM UTC · Completed 3:21 AM UTC
Commit: 47bb282 · View workflow run →

[waynesun09]

Review squad pass (3 agents: Claude coder, Claude researcher, Grok). All prior fixes verified correct. 7 new MEDIUM findings posted inline, plus 1 that couldn't be attached inline:

[MEDIUM] ADR 0057 Implementation Status is stale — The ADR (not in this PR's diff) still says repos install "is being implemented in PR #3033" and lists "Remaining subcommands" without mentioning repos add, repos remove, or repos uninstall. Update the status section to reflect what's now implemented.

No HIGH or CRITICAL findings. The destructive teardown ordering and partial-failure handling are functionally sound — the findings are about operator UX (confirmation text, dry-run preview, error reporting) and defensive input validation (glob errors, guard-variable deletion order).

Assisted-by: Claude (review), Grok (review)

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:25 PM UTC · Ended 1:30 PM UTC
Commit: cc7a526 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:31 PM UTC · Ended 1:41 PM UTC
Commit: cc7a526 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:42 PM UTC · Completed 1:56 PM UTC
Commit: fb63920 · View workflow run →

[waynesun09]

Verification squad pass (3 agents: Claude coder, Claude researcher, Grok). All 7 MEDIUM findings from the prior round are verified fixed in the current head:

1. Secret deletion error — errors.Join combines both errors ✓
2. Confirmation prompt — "remove and uninstall" when --uninstall set ✓
3. Guard variable order — guard deleted first ✓
4. filepath.Match error — propagated through all primary callers ✓
5. --yes help text — "when multiple repos are targeted" in both commands ✓
6. --dry-run uninstall preview — calls Uninstall with DryRun: true ✓
7. ADR status — out of scope for this PR (tracked in stack)

Build clean, all tests pass. No new MEDIUM+ findings — only LOW test-coverage gaps (combined --dry-run --uninstall path, dual deletion failure path).

Assisted-by: Claude (review), Grok (review)

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 5:24 PM UTC · Completed 5:42 PM UTC
Commit: fb63920 · View workflow run →

[fullsend-ai-retro]

Retro: PR #4081 — repos management subcommands (ADR-0057 PR 8)

Timeline

This human-authored PR by @ggallen added four CLI subcommands (repos add, repos remove, repos uninstall, updated repos install) across 21 changed files (+3403/-212 lines). It went through 18 workflow runs (7 successful reviews, 4 failures, 6 cancelled, 1 retro) over 7 days (Jul 10-17).

• Jul 10: Initial push. Review agent (run 29126582718) found 6 findings (4 medium, 2 low): silent WIF type-assertion failure, missing confirmation prompt, scope creep vs plan doc, missing CLI docs, implicit behavioral change, and input validation gap.
• Jul 10-11: 4 runs failed with GitHub API 422 errors during review submission (already tracked by fullsend#5140, agents#193).
• Jul 11: @ggallen fixed all 6 agent findings. Review agent approved.
• Jul 13: Human reviewer @waynesun09 ran a multi-model review squad (Claude, Claude, Gemini) and found 2 HIGH + 1 MEDIUM findings the review agent had missed entirely: partial-failure manifest inconsistency, missing confirmation for explicit bulk lists, and manifest-before-install with no rollback.
• Jul 17: @ggallen fixed those 3 findings. @waynesun09 ran a second squad pass (Claude coder, Claude researcher, Grok) and found 7 additional MEDIUM findings: secret deletion error dropped, confirmation prompt wording, guard variable deletion order, filepath.Match error discarded, help text scope mismatch, and dry-run preview incompleteness.
• Jul 17: @ggallen fixed all 7. @waynesun09 ran a verification pass confirming all fixes. Review agent approved. PR merged.

Review quality assessment

The review agent and human-directed squad showed complementary but non-overlapping strengths:

Reviewer	Findings	Severity range	Focus area
fullsend-ai-review	6 initial + 3 later rounds	medium-low	Code architecture: type assertions, interface design, input validation, scope coherence
waynesun09's squad	2 HIGH + 8 MEDIUM	high-medium	Operational safety: partial failure handling, confirmation UX, error propagation, dry-run completeness

The review agent's initial pass missed all 10 operational safety findings that the human-directed squad later caught. These findings are characterized by reasoning about "what happens when things go wrong in production" rather than "is the code logically correct." The review agent's correctness sub-agent checks for error handling gaps generically but lacks specific patterns for CLI operational safety.

Items already tracked by existing issues

• 422 review submission failures (4 runs wasted): Tracked by fullsend#5140, fullsend#2569, agents#193
• Severity escalation for medium findings: Broadly tracked by fullsend#1453, fullsend#2940, agents#33
• Go concurrent fan-out error consolidation guidance (for code agents): Tracked by fullsend#3512

Proposals filed (3)

The three proposals below target the review agent's correctness sub-agent in fullsend-ai/agents, addressing the systematic gap in CLI operational safety detection that this PR exposed.

Proposals filed

• Correctness sub-agent: add batch-operation partial-failure consistency checks (in fullsend-ai/agents)
• Correctness sub-agent: detect parallel error variable consolidation gaps (in fullsend-ai/agents)
• Correctness sub-agent: add CLI operational safety checklist for destructive commands (in fullsend-ai/agents)