feat(repos): add upgrade and upgrade-mint subcommands

[ggallen]

Summary

• Implement fullsend repos upgrade to upgrade scaffold shim refs across repos in a manifest, with semver comparison, floating ref detection, --force/--dry-run/--ref flags, positional args for repo filtering, and bounded concurrency.
• Implement fullsend repos upgrade-mint to verify the mint deployment matches the manifest configuration.
• PR 7 of the ADR 0057 repos-management implementation plan.

Note: This PR adds entirely new subcommands (upgrade, upgrade-mint) that have never been released. No ! breaking-change suffix is needed — there is no existing behavior to break.

Changes

File	Action
internal/repos/upgrade.go	Create — Upgrade(), UpgradeMint(), replaceShimRef(), semver helpers
internal/repos/upgrade_test.go	Create — 45 tests covering upgrade logic, ref replacement, semver comparison, floating ref detection
internal/cli/repos.go	Modify — wire repos upgrade and repos upgrade-mint commands
internal/cli/repos_test.go	Modify — 16 tests for flag registration, shorthands, subcommand wiring, and integration
internal/repos/manifest.go	Modify — add IsValidRef validation in Validate() for defaults.fullsend_ref and per-repo fullsend_ref
internal/repos/manifest_test.go	Modify — add validation tests for invalid refs
docs/ADRs/0057-repos-management.md	Modify — update Implementation Status, fix upgrade-mint description
docs/plans/repos-management.md	Modify — mark PR 7 complete, update upgrade/upgrade-mint descriptions to match implementation, fix ProvisionerFactory type signature
docs/cli/repos.md	Modify — add upgrade and upgrade-mint command documentation
docs/guides/dev/cli-internals.md	Modify — add upgrade/upgrade-mint to CLI tree
docs/guides/getting-started/operations.md	Modify — add upgrade/upgrade-mint to operations table

ADR edits

ADR 0057 (Accepted) is modified to:

• Update Implementation Status: marks repos upgrade and repos upgrade-mint as implemented in PR feat(repos): add upgrade and upgrade-mint subcommands #4080
• Fix upgrade-mint subcommand description from "Upgrade token mint Cloud Function" to "Verify token mint deployment against manifest" (matches actual implementation)

CLI pattern alignment (PR #4081)

Follows the patterns established by PR #4081 (repos add, repos remove, repos uninstall):

• reposUpgradeConfig / reposUpgradeMintConfig structs with testClient / testProvisioner fields for test injection
• Positional args for repo filtering (upgrade [repos...]) instead of --repo flag
• Run functions take config struct pointers: runReposUpgrade(ctx, opts, repoFilter)
• CLI-level tests for flags, shorthands, subcommand wiring, and integration via test hooks

Test plan

• 45 upgrade-related tests pass (go test ./internal/repos/)
• 16 CLI upgrade tests pass (go test ./internal/cli/ -run TestReposUpgrade)
• Full repos and CLI package test suites pass
• go build ./... compiles cleanly
• go vet passes
• Pre-commit hooks pass

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:50 PM UTC · Completed 10:02 PM UTC
Commit: aeaa431 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Add repos upgrade and upgrade-mint CLI subcommands

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add fullsend repos upgrade to batch-update scaffold workflow refs across manifest repos.
• Skip floating refs and prevent downgrades unless --force is set.
• Add fullsend repos upgrade-mint to verify mint deployment matches manifest configuration.

Diagram

graph TD
  A["fullsend CLI"] --> B["internal/cli/repos.go"] --> C["repos.yaml manifest"] --> D["repos.Upgrade"] --> E["forge.Client"] --> F{{"GitHub API"}}
  C --> G["repos.UpgradeMint"] --> H{{"GCF mint deployment"}}

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use a semver library (e.g., golang.org/x/mod/semver)

• ➕ Correct handling of pre-release/build metadata ordering
• ➕ Less bespoke regex/parsing code to maintain
• ➖ Adds/expands dependency surface (if not already used elsewhere)
• ➖ May require normalizing refs that don’t strictly follow semver

2. Parse workflow YAML and rewrite only `uses:` nodes

• ➕ Avoids regex-based false positives/negatives
• ➕ More precise updates when workflows have multiple uses: blocks
• ➖ YAML round-tripping can reorder/format files, increasing diff noise
• ➖ More implementation complexity than simple line replacement

Recommendation: Current approach is reasonable for the goal (targeted replacement of fullsend-ai/fullsend/...@ref plus conservative skipping rules) and is well-covered by tests. If pre-release ordering or broader ref formats become important, consider swapping the custom semver comparison for a standard semver library; if replacement accuracy becomes an issue, consider YAML-aware rewriting at the cost of formatting churn.

Files changed (3) +1487 / -0

Enhancement (2) +497 / -0

repos.goWire new 'repos upgrade' and 'repos upgrade-mint' Cobra commands +188/-0

Wire new 'repos upgrade' and 'repos upgrade-mint' Cobra commands

• Adds two new subcommands with flags for manifest path, ref override, repo filtering, dry-run/force behavior, and bounded concurrency. Implements command runners that load/validate the manifest, construct clients/adapters, invoke 'repos.Upgrade'/'repos.UpgradeMint', and summarize results.

internal/cli/repos.go

upgrade.goImplement batch scaffold ref upgrade + mint verification logic +309/-0

Implement batch scaffold ref upgrade + mint verification logic

• Introduces a concurrent upgrade engine that reads workflow files, extracts current refs, skips floating refs, blocks downgrades via semver comparison unless forced, and rewrites 'uses: fullsend-ai/fullsend/...@ref' occurrences. Adds 'UpgradeMint' to discover the mint deployment URL and validate it matches the manifest.

internal/repos/upgrade.go

Tests (1) +990 / -0

upgrade_test.goAdd comprehensive tests for upgrade and mint verification behavior +990/-0

Add comprehensive tests for upgrade and mint verification behavior

• Adds a large suite of tests covering upgrade scenarios (behind/current/ahead, force, dry-run, ref overrides, repo filtering, workflow path variants, error handling, and callbacks) plus direct tests for ref replacement and semver/floating-ref helpers. Includes tests for 'UpgradeMint' success and failure modes.

internal/repos/upgrade_test.go

[codecov]

Codecov Report

❌ Patch coverage is 87.53623% with 43 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/repos.go	75.73%	24 Missing and 9 partials ⚠️
internal/repos/upgrade.go	95.12%	5 Missing and 5 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. --direct flag ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

repos.Upgrade() always calls the scaffold commit callback with direct=true, so `fullsend repos
upgrade --direct=false` still attempts direct pushes and cannot force PR-based delivery. The CLI
parses/passes a direct flag, but it never influences the upgrade logic.

Code

internal/repos/upgrade.go[R193-194]

+	if err := commitFn(ctx, owner, repo, files, true); err != nil {
+		result.Error = fmt.Errorf("committing upgrade: %w", err)

Relevance

⭐⭐⭐ High

Team fixes CLI flag mismatches/PR-vs-direct delivery issues (accepted in PR #697, PR #2630).

PR-#697
PR-#2630

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The upgrade code hardcodes direct=true when committing, while the CLI exposes a --direct flag
that never affects the upgrade config; CommitScaffoldFiles explicitly switches PR vs direct
behavior based on this boolean.

internal/repos/upgrade.go[185-196]
internal/cli/repos.go[495-579]
internal/layers/commit.go[15-36]

Agent prompt

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

## Issue description
`fullsend repos upgrade --direct=false` cannot work because the upgrade path hardcodes `direct=true` when calling the commit callback.

## Issue Context
- The CLI defines a `--direct` flag and passes it into `runReposUpgrade(...)`, but the value is dropped before reaching `repos.Upgrade()`.
- `layers.CommitScaffoldFiles` uses the `direct` boolean to decide between direct-to-default-branch vs PR delivery.

## Fix Focus Areas
- internal/repos/upgrade.go[106-201]
- internal/cli/repos.go[495-579]
- internal/layers/commit.go[15-36]

## Implementation notes
- Add a `Direct bool` field to `repos.UpgradeConfig` (or pass `direct` as an explicit parameter) and use it in `upgradeRepo()` when calling `commitFn(..., direct)`.
- Wire `runReposUpgrade(..., direct)` into the upgrade config.
- Add/adjust tests in `internal/repos/upgrade_test.go` to assert the commitFn receives `direct=false` when requested.

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

---

2. Prerelease downgrades allowed ✓ Resolved 🐞 Bug ≡ Correctness

Description

The downgrade protection can be bypassed for prerelease tags because compareSemver ignores
prerelease suffixes (e.g., v2.3.0 vs v2.3.0-rc1 compares equal). This allows moving from a final
release to an RC without --force, contradicting the CLI’s “downgrades are blocked” behavior.

Code

internal/repos/upgrade.go[R243-269]

+// isSemver returns true if the ref looks like a semver version tag (vX.Y.Z with optional pre-release).
+var semverPattern = regexp.MustCompile(`^v(\d+)\.(\d+)\.(\d+)`)
+
+func isSemver(ref string) bool {
+	return semverPattern.MatchString(ref)
+}
+
+// compareSemver compares two semver refs (vX.Y.Z format).
+// Returns -1 if a < b, 0 if a == b, 1 if a > b.
+// Only compares major.minor.patch; pre-release suffixes are ignored.
+func compareSemver(a, b string) int {
+	am := semverPattern.FindStringSubmatch(a)
+	bm := semverPattern.FindStringSubmatch(b)
+	if am == nil || bm == nil {
+		return 0
+	}
+	for i := 1; i <= 3; i++ {
+		av := parseUint(am[i])
+		bv := parseUint(bm[i])
+		if av < bv {
+			return -1
+		}
+		if av > bv {
+			return 1
+		}
+	}
+	return 0

Relevance

⭐⭐⭐ High

Team has prior accepted fixes around prerelease/version semantics (PR #790); likely to tighten
downgrade protection too.

PR-#790

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CLI documents downgrade blocking, but the implementation’s semver comparator explicitly ignores
prerelease suffixes; the downgrade check only triggers when compareSemver returns > 0, which won’t
happen for release vs prerelease with the same major/minor/patch.

internal/cli/repos.go[509-515]
internal/repos/upgrade.go[161-167]
internal/repos/upgrade.go[243-269]

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 downgrade guard relies on `compareSemver(currentRef, targetRef) > 0`, but `compareSemver` ignores prerelease suffixes, making `vX.Y.Z` and `vX.Y.Z-rcN` compare as equal.

## Issue Context
- The CLI help states downgrades are blocked unless `--force` is set.
- `isSemver()` deliberately treats prereleases like `v2.3.0-rc1` as semver, so this path is reachable.

## Fix Focus Areas
- internal/repos/upgrade.go[161-167]
- internal/repos/upgrade.go[243-270]
- internal/repos/upgrade_test.go[691-742]

## Implementation notes
- Replace the custom regex-based semver parsing with `golang.org/x/mod/semver` (`semver.IsValid`, `semver.Compare`) so prerelease ordering is respected.
- Ensure the comparison logic treats `v2.3.0-rc1` as less than `v2.3.0` so downgrades to prereleases are blocked unless `--force` is set.
- Update the unit tests to cover the release-vs-prerelease ordering case explicitly (e.g., `v2.3.0` > `v2.3.0-rc1`).

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

---

[fullsend-ai-review]

Review

Re-review of 15207a0 (prior review at 6e05ef7, provenance: app-verified).

All 11 files changed since prior review (1 commit). Prior review's low findings re-evaluated — both remain valid against unchanged patterns. No new medium+ findings. Security review confirmed all ref injection surfaces are covered by IsValidRef at three layers (CLI flag, manifest validation, per-repo execution), $ escaping in replaceShimRef prevents regex backreference injection, and checkPerRepoScopes is correctly called for write operations.

Low

[edge-case] internal/repos/upgrade.go — shimRefPattern matches commented-out uses: lines

The shimRefPattern regex matches uses: anywhere on a line, including inside YAML comments (e.g., # uses: fullsend-ai/fullsend/...@v1.0.0). If a workflow file contains a commented-out fullsend uses line, replaceShimRef will modify the commented line as well. The impact is benign (updating a comment's ref) and the pattern is narrow enough that false positives in non-comment contexts are unlikely.

[silent-skip] internal/repos/upgrade.go — OldRef empty when only action uses: lines present

When a workflow file contains fullsend action uses: lines (e.g., .github/actions/mint-token@ref) but no fullsend workflow uses: lines, extractWorkflowRef returns an empty string. The upgrade proceeds correctly via replaceShimRef, but UpgradeResult.OldRef will be empty. This edge case is unlikely in practice since repos always have a workflow uses: line.

Strengths

• Prior review's high [cli-breaking-change] finding correctly resolved in prior cycle — this PR only adds new commands; no existing interface is broken.
• Comprehensive test coverage: 45 upgrade tests covering edge cases, floating refs, semver comparison (including prerelease §11), mixed-ref scenarios, and mint verification. 16 CLI tests for flag registration, validation, and integration.
• IsValidRef provides strict allowlist defense at three layers: CLI flag, manifest validation, per-repo execution.
• $ escaping in replaceShimRef prevents regex backreference injection.
• Semver prerelease comparison correctly implements §11 with proper numeric vs string handling.
• Build metadata correctly excluded per semver 2.0.0 §10.
• parseUint overflow guard prevents silent wrap-around on crafted version numbers.
• shimRefPattern uses [ \t]* (not \s*) for the comment group to avoid matching across newlines.
• repos upgrade-mint is verification-only (read-only DiscoverMint), appropriate security posture.
• checkPerRepoScopes correctly called for upgrade (write operations); correctly omitted for upgrade-mint (read-only).
• Documentation for both commands present in docs/cli/repos.md, docs/guides/dev/cli-internals.md, and docs/guides/getting-started/operations.md.
• ADR 0057 Implementation Status appropriately updated with minor annotations.
• No security findings: token handling follows established patterns, forge abstraction respected.

Previous run

Review

Re-review of 6e05ef7 (prior review at 7bf3b2d, provenance: app-verified).

Prior review's high [cli-breaking-change] finding re-evaluated and resolved: the --repo flag removal from repos install occurred in PR #4081, not in this PR. This PR only adds the new repos upgrade and repos upgrade-mint subcommands — no existing interface is changed. Prior review's low [stale-doc] finding on plan document resolved — stale signatures corrected in this iteration.

Low

[edge-case] internal/repos/upgrade.go — shimRefPattern matches commented-out uses: lines

The shimRefPattern regex matches uses: anywhere on a line, including inside YAML comments (e.g., # uses: fullsend-ai/fullsend/...@v1.0.0). If a workflow file contains a commented-out fullsend uses line, replaceShimRef will modify the commented line as well. The impact is benign (updating a comment's ref) and the pattern is narrow enough that false positives in non-comment contexts are unlikely.

[silent-skip] internal/repos/upgrade.go — OldRef empty when only action uses: lines present

When a workflow file contains fullsend action uses: lines (e.g., .github/actions/mint-token@ref) but no fullsend workflow uses: lines, extractWorkflowRef returns an empty string. The upgrade proceeds correctly via replaceShimRef, but UpgradeResult.OldRef will be empty. This edge case is unlikely in practice since repos always have a workflow uses: line.

[scope-boundary] docs/plans/repos-management.md — upgrade-mint scope reduced to verification only

The plan originally described upgrade-mint as "Upgrades the token mint Cloud Function" but the implementation only verifies the mint URL matches the manifest. The plan's PR 7 section documents this deferral with rationale.

Strengths

• Prior review's high [cli-breaking-change] finding correctly resolved — this PR only adds new commands; no existing interface is broken.
• Prior review's low [stale-doc] finding resolved — plan document signatures updated.
• Comprehensive test coverage: 30+ upgrade tests covering edge cases, floating refs, semver comparison, prerelease handling, mixed-ref scenarios, and mint verification.
• IsValidRef provides strict allowlist defense at three layers: CLI flag, manifest validation, per-repo execution.
• $ escaping in replaceShimRef prevents regex backreference injection.
• Semver prerelease comparison correctly implements §11 with proper numeric vs string handling.
• Build metadata correctly excluded per semver 2.0.0 §10.
• parseUint overflow guard prevents silent wrap-around on crafted version numbers.
• shimRefPattern uses [ \t]* (not \s*) for the comment group to avoid matching across newlines.
• repos upgrade-mint is verification-only (read-only DiscoverMint), appropriate security posture.
• No security findings: token handling follows established patterns, forge abstraction respected.
• Documentation for both commands present in docs/cli/repos.md, docs/guides/dev/cli-internals.md, and docs/guides/getting-started/operations.md.

Previous run (2)

Review

Re-review of 7bf3b2d (prior review at 50f7c62, provenance: app-verified).

Head SHA changed from 50f7c62 to 7bf3b2d. 28 files changed since prior review. Prior review's medium [error-handling-gap] finding on init.go has been resolved — the code now logs a warning via the progress callback when ProbeRepoState returns an error for an installed repo. Prior review's medium [silent-skip] finding on sync.go has been downgraded to low — the error IS surfaced in SyncResult.Warnings, just not counted in Failed. The prior review's high-severity finding about the PR title missing ! suffix remains unaddressed.

High

[cli-breaking-change] PR title missing ! suffix for breaking change

The --repo flag has been removed from repos install and replaced with positional arguments. A test explicitly asserts --repo flag should be removed, use positional args. The PR title is feat(repos): add upgrade and upgrade-mint subcommands — missing !. Per COMMITS.md: "Breaking changes must carry the ! suffix in both commit messages and PR titles." GoReleaser uses PR titles to build release notes, so users will not see the breaking change warning.

Remediation: Update the PR title to include the ! suffix, e.g. feat(repos)!: add repos management commands (add, remove, install, uninstall, diff, sync, upgrade, upgrade-mint).

Low

[silent-skip] internal/repos/sync.go — API errors not counted in failure total

When diffRepo returns ok==false due to an API error (e.g., ListRepoVariables failure), the repo is not counted in SyncResult.Failed. The error IS included in SyncResult.Warnings, so the information is not lost, but a caller checking Failed==0 && err==nil would conclude sync fully succeeded. Downgraded from medium because warnings are surfaced.

[scope-transparency] PR title and body do not reflect actual scope

The PR title says "add upgrade and upgrade-mint subcommands" but the PR delivers 8 subcommands: add, remove, install (breaking change), uninstall, diff, sync, upgrade, upgrade-mint. The implementation plan authorizes the broader scope.

[scope-boundary] docs/plans/repos-management.md — upgrade-mint scope reduced to verification only

The plan originally described upgrade-mint as "Upgrades the token mint Cloud Function" but the implementation only verifies the mint URL matches the manifest. The plan's PR 7 section documents this deferral with rationale.

[stale-doc] docs/plans/repos-management.md — stale function signatures and references

The plan contains stale references: ProvisionerFactory type signature (uses InstallConfig, implementation uses ResolvedConfig), replaceShimRef signature (missing newTag parameter, shows error return instead of bool), WIFProvisioner interface missing DeleteWIFProvider, and description claims replaceShimRef updates input values (it only replaces @ref in uses: lines).

Strengths

• Prior review's blocking [error-handling-gap] finding resolved: discoverRepo now logs a warning via the progress callback when ProbeRepoState returns an error for an installed repo.
• Comprehensive test coverage: 30+ upgrade tests, 40+ uninstall tests, 30+ sync/diff tests, 15+ manifest edit tests.
• ProbeRepoState refactor eliminates code duplication between discoverRepo and the new commands.
• Two-phase uninstall (parallel cleanup → sequential WIF) correctly handles the read-modify-write constraint on mint env vars.
• IsValidRef provides strict allowlist defense at three layers: CLI flag, manifest validation, per-repo execution.
• $ escaping in replaceShimRef prevents regex backreference injection.
• Semver prerelease comparison correctly implements §11 with proper numeric vs string handling.
• Secrets handling properly separates non-sensitive variables (logged in cleartext) from sensitive secrets (never displayed).
• Glob matching via filepath.Match is safe against ReDoS and correctly integrated with confirmation prompts for destructive operations.
• WIF Provider ID construction chain well-defended: SplitN validation → ToLower → BuildRepoProviderID → url.PathEscape.
• No security findings: token handling follows established patterns, forge abstraction respected throughout.

Previous run (3)

Review

Re-review of 50f7c62 (prior review at 5fec1bc, provenance: app-verified).

Head SHA changed from 5fec1bc to 50f7c62. 11 files changed since prior review (docs, CLI wiring, manifest validation, upgrade code/tests). The prior review's high-severity finding about the PR title missing ! suffix remains unaddressed.

High

[breaking-cli] PR title missing ! suffix for breaking change

Commit 1 is feat(repos)!: add, remove, install, uninstall subcommands — the ! indicates a breaking change (removes --repo flag from repos install, replaces with positional args). A test explicitly asserts --repo flag should be removed, use positional args. The PR title is feat(repos): add upgrade and upgrade-mint subcommands — missing !. Per COMMITS.md: "Breaking changes must carry the ! suffix in both commit messages and PR titles; a missing ! is an important-severity review finding." GoReleaser uses PR titles to build release notes, so users will not see the breaking change warning.

Remediation: Update the PR title to include the ! suffix, e.g. feat(repos)!: add repos management commands (add, remove, install, uninstall, diff, sync, upgrade, upgrade-mint).

Medium

[error-handling-gap] internal/repos/init.go — discoverRepo silently swallows workflow-read errors

The refactored discoverRepo uses ProbeRepoState which returns (state{Installed:true}, error) when the guard variable is set but readWorkflowRef fails (e.g., API error reading the workflow file). The guard if err != nil && !state.Installed evaluates to false when Installed=true, so the error is discarded. Discovery proceeds with an empty FullsendRef. The original code returned the error as a hard failure via readWorkflowRef. This behavioral regression could cause the init command to produce incomplete manifest entries without reporting the underlying issue.

Remediation: After the guard check, add a separate handler for err != nil && state.Installed: either log via progress callback (progress(fullName, "discover", fmt.Sprintf("warning: %v", err))) or return the error to mark the discovery as failed.

[silent-skip] internal/repos/sync.go — Sync silently skips secret convergence on API error

When diffRepo returns warnings from an API error (e.g., ListRepoVariables failure), len(changes)==0 and len(diffWarnings)>0. The code path skips ensureSecrets entirely and does not set res.failed=true. The repo is counted only in SyncResult.Warnings, not SyncResult.Failed. A caller checking Failed==0 && err==nil would believe sync fully succeeded, even though secrets were never written. Since ensureSecrets uses only manifest-resolved config (not variable reads), it could safely run independently.

Remediation: Either always attempt ensureSecrets regardless of diffWarnings (since secret writes don't depend on variable reads), or set res.failed=true when diffWarnings are non-empty so the failure count reflects the skipped repo.

Low

[scope-transparency] PR title and body do not reflect actual scope

The PR title says "add upgrade and upgrade-mint subcommands" but the PR delivers 8 new/modified subcommands across three commits: add, remove, install (breaking change), uninstall, diff, sync, upgrade, upgrade-mint. The implementation plan (docs/plans/repos-management.md) authorizes the broader scope.

[design-deviation] docs/plans/repos-management.md — upgrade-mint scope reduced to verification only

The plan states "Upgrades the token mint Cloud Function" but the implementation only verifies the mint URL matches the manifest. The plan's PR 7 section documents this deferral with rationale ("verification only — full redeploy deferred until /health version endpoint is available"), but the top-level command spec has not been updated to match.

[design-deviation] docs/plans/repos-management.md — upgrade uses regex replacement, not scaffold regeneration

The plan spec says "Uses ADR 0048's --upstream-ref" for scaffold regeneration, but the implementation uses replaceShimRef regex replacement. The plan's implementation section discusses this alternative but the primary command spec should be updated to reflect the as-built approach.

[interface-extension] internal/repos/install.go — WIFProvisioner interface extended with DeleteWIFProvider

New method DeleteWIFProvider(ctx context.Context, repo string) error added to the exported WIFProvisioner interface. The interface is in an internal/ package so external implementations are not possible. All in-tree implementations are updated.

[incomplete-destructive-ordering] internal/cli/repos.go — chained destructive operations in DeletePerRepoWIF

splitProjectAdapter.DeletePerRepoWIF chains two destructive operations: (1) mint deregistration and (2) WIF provider deletion. If step 2 fails after step 1 succeeds, creates a partially-torn-down state. The error message correctly notes "mint deregistration already succeeded — re-run is safe."

[adr-annotation] docs/ADRs/0057-repos-management.md — Implementation Status section incomplete

The status update claims only upgrade/upgrade-mint are implemented in PR #4080. The PR also implements repos add, remove, uninstall, diff, and sync. The "Remaining" list still includes repos sync and repos remove, which are delivered in this PR.

[scope-boundary] docs/ADRs/0057-repos-management.md — table row changes upgrade-mint purpose

The ADR table row was changed from "Upgrade token mint Cloud Function" to "Verify token mint deployment against manifest." This alters the recorded decision's scope in an Accepted ADR. Per AGENTS.md, substantive changes to Accepted ADRs should be made via a new superseding ADR.

[naming-consistency] docs/plans/repos-management.md — remove/uninstall command split

The plan describes infrastructure teardown under repos remove, but the implementation splits this into repos remove (manifest editing only) and repos uninstall (infrastructure teardown). The split improves safety but the plan's command table should be updated.

[stale-doc] docs/plans/repos-management.md — stale function signatures and references

The plan contains stale references: ProvisionerFactory type signature (uses InstallConfig, implementation uses ResolvedConfig), replaceShimRef signature (missing newTag parameter, shows error return instead of bool), and test strategy references to the removed --repo flag.

Strengths

• Prior review's blocking findings (compilation error, stale docs) all resolved across earlier iterations.
• Comprehensive test coverage: 30+ upgrade tests, 40+ uninstall tests, 30+ sync/diff tests, 15+ manifest edit tests.
• ProbeRepoState refactor eliminates code duplication between discoverRepo and the new commands.
• Two-phase uninstall (parallel cleanup → sequential WIF) correctly handles the read-modify-write constraint on mint env vars.
• Glob matching via filepath.Match is safe against ReDoS and correctly integrated with confirmGlobAction for destructive operations.
• WIF Provider ID construction chain well-defended: SplitN validation → ToLower → BuildRepoProviderID → url.PathEscape.
• Secrets handling properly separates non-sensitive variables (logged in cleartext) from sensitive secrets (never displayed).
• Manifest validation (Validate()) now validates fullsend_ref values with IsValidRef at both defaults and per-repo levels.
• $ escaping in replaceShimRef prevents regex backreference injection.
• Semver prerelease comparison correctly implements §11 with proper numeric vs string handling.
• IsValidRef provides strict allowlist defense at three layers: CLI flag, manifest validation, per-repo execution.
• No security findings: token handling follows established patterns, forge abstraction respected throughout.

Previous run (4)

Review

Re-review of 5fec1bc (prior review at abb3dc7, provenance: app-verified).

Head SHA changed from abb3dc7 to 5fec1bc (rebase onto main). The prior review's high-severity finding about the PR title missing ! suffix remains unaddressed. Two new medium-severity findings identified on re-review.

High

[breaking-cli] PR title missing ! suffix for breaking change

Commit 1 is feat(repos)!: add, remove, install, uninstall subcommands — the ! indicates a breaking change (removes --repo flag from repos install, replaces with positional args). The PR title is feat(repos): add upgrade and upgrade-mint subcommands — missing !. Per COMMITS.md: "Breaking changes must carry the ! suffix in both commit messages and PR titles; a missing ! is an important-severity review finding." GoReleaser uses PR titles to build release notes, so users will not see the breaking change warning.

Remediation: Update the PR title to include the ! suffix, e.g. feat(repos)!: add repos management commands (add, remove, install, uninstall, diff, sync, upgrade, upgrade-mint).

Medium

[error-handling-gap] internal/repos/init.go — discoverRepo silently swallows workflow-read errors

The refactored discoverRepo uses ProbeRepoState which returns (state{Installed:true}, error) when the guard variable is set but readWorkflowRef fails (e.g., API error reading the workflow file). The guard if err != nil && !state.Installed evaluates to false when Installed=true, so the error is discarded. Discovery proceeds with an empty FullsendRef. The original code returned the error as a hard failure via readWorkflowRef. This behavioral regression could cause the init command to produce incomplete manifest entries without reporting the underlying issue.

Remediation: Either propagate the error (the original behavior) or report via the progress callback: if err != nil && state.Installed { progress(fullName, "discover", fmt.Sprintf("warning: %v", err)) }.

[breaking-interface] internal/repos/install.go — WIFProvisioner interface extended with DeleteWIFProvider

New method DeleteWIFProvider(ctx context.Context, repo string) error added to the exported WIFProvisioner interface. All internal implementations are updated. The interface is in an internal/ package so external implementations are not possible, but the BREAKING CHANGE: trailer should document this specific interface change alongside the CLI flag removal.

Low

[scope-transparency] PR title and body do not reflect actual scope

The PR title says "add upgrade and upgrade-mint subcommands" but the PR delivers 8 new/modified subcommands across three commits: add, remove, install (breaking change), uninstall, diff, sync, upgrade, upgrade-mint. The implementation plan (docs/plans/repos-management.md) authorizes the broader scope.

[incomplete-destructive-ordering] internal/cli/repos.go — chained destructive operations in DeletePerRepoWIF

splitProjectAdapter.DeletePerRepoWIF chains two destructive operations: (1) mint.DeletePerRepoWIF (deregisters from mint) and (2) inference.DeleteWIFProvider (deletes GCP IAM WIF provider). If step 2 fails after step 1 succeeds, creates a partially-torn-down state. The error message correctly notes "mint deregistration already succeeded — re-run is safe."

[edge-case] internal/repos/upgrade.go — OldRef reporting scope mismatch

replaceShimRef matches all fullsend-ai/fullsend/ paths (workflows + actions) while extractWorkflowRef only matches .github/workflows/ paths. OldRef in UpgradeResult only reflects the workflow ref, not action refs, which could be misleading when only the action ref needed upgrading.

[input-validation] internal/repos/manifest_edit.go — writeManifest path not canonicalized

writeManifest writes to a caller-controlled path argument (from --manifest flag, default repos.yaml) without path canonicalization. Since this is a locally-run CLI where the user controls all inputs, this is a minor defense-in-depth observation.

[ref-injection-defense-in-depth] internal/repos/upgrade.go — replaceShimRef lacks self-validation

replaceShimRef does not call IsValidRef itself, relying on callers (upgradeRepo validates refs before calling). Adding a defensive check in replaceShimRef would make it safe regardless of caller context.

[concurrency-pattern] internal/repos/uninstall.go — goroutine/semaphore pattern divergence

Uninstall spawns goroutines before acquiring the semaphore (matching batch_install.go and init.go), while Sync, Upgrade, and Status acquire the semaphore first. Both patterns are correct; this is a package-wide inconsistency, not a new divergence introduced by this PR.

[error-message-consistency] internal/repos/upgrade.go — error message capitalization

validateConcurrency (used by sync.go and upgrade.go) produces "concurrency must be between 1 and 32" while uninstall.go and batch_install.go use "MaxConcurrency must be between 1 and 32". Minor cross-file inconsistency in the package.

Strengths

• Prior review's blocking findings (fakeProvisioner compilation error, stale docs) all resolved across earlier iterations.
• Comprehensive test coverage: 30+ upgrade tests, 40+ uninstall tests, 30+ sync/diff tests, 15+ manifest edit tests.
• ProbeRepoState refactor eliminates code duplication between discoverRepo and the new commands.
• Two-phase uninstall (parallel cleanup → sequential WIF) correctly handles the read-modify-write constraint on mint env vars.
• Glob matching via filepath.Match is safe against ReDoS and correctly integrated with confirmGlobAction for destructive operations.
• WIF Provider ID construction chain well-defended: SplitN validation → ToLower → BuildRepoProviderID → url.PathEscape.
• Secrets handling properly separates non-sensitive variables (logged in cleartext) from sensitive secrets (never displayed).
• Manifest validation (Validate()) now validates fullsend_ref values with IsValidRef at both defaults and per-repo levels.
• $ escaping in replaceShimRef prevents regex backreference injection.
• Semver prerelease comparison correctly implements §11 with proper numeric vs string handling.
• Documentation for all new commands present in docs/cli/repos.md, docs/guides/dev/cli-internals.md, and docs/guides/getting-started/operations.md.
• ADR 0057 Implementation Status section appropriately updated with minor annotations.
• No security findings: token handling follows established patterns, IsValidRef provides strict allowlist defense at three layers, forge abstraction respected throughout.

Previous run (5)

Review

Re-review of abb3dc7 (prior review at 4f06327, provenance: app-verified).

The head SHA changed from 4f06327 to abb3dc7 (rebase of the third commit). No substantive code changes since the prior review. The prior review's blocking compilation error (fakeProvisioner missing DeleteWIFProvider) remains resolved. The prior stale-doc findings (CLI tree, ProvisionerFactory type signature) remain resolved. The PR body now includes an "## ADR edits" section, addressing the prior review's [adr-amendment-scope] finding.

One high-severity finding from the prior review remains unaddressed: the PR title is still missing the ! suffix for the breaking change.

High

[breaking-cli] PR title missing ! suffix for breaking change

Commit 1 is feat(repos)!: add, remove, install, uninstall subcommands — the ! indicates a breaking change (removes --repo flag from repos install, replaces with positional args). A test explicitly asserts --repo flag should be removed, use positional args. The PR title is feat(repos): add upgrade and upgrade-mint subcommands — missing !. Per COMMITS.md: "Breaking changes must carry the ! suffix in both commit messages and PR titles; a missing ! is an important-severity review finding." GoReleaser uses PR titles to build release notes, so users will not see the breaking change warning.

Remediation: Update the PR title to include the ! suffix, e.g. feat(repos)!: add repos management commands (add, remove, install, uninstall, diff, sync, upgrade, upgrade-mint).

Low

[scope-creep] PR title and body do not reflect actual scope

The PR title says "add upgrade and upgrade-mint subcommands" and the body describes itself as "PR 7" of the implementation plan, but the PR actually delivers 8 new/modified subcommands across content from implementation plan PRs 1, 2, 6, and 7: add, remove, install (breaking change), uninstall, diff, sync, upgrade, upgrade-mint.

[incomplete-destructive-ordering] internal/cli/repos.go — chained destructive operations in DeletePerRepoWIF

splitProjectAdapter.DeletePerRepoWIF chains two destructive operations: (1) mint.DeletePerRepoWIF (deregisters from mint) and (2) inference.DeleteWIFProvider (deletes GCP IAM WIF provider). If step 2 fails after step 1 succeeds, creates a partially-torn-down state with an orphaned WIF provider. The error message correctly notes "mint deregistration already succeeded — re-run is safe", and the error surfaces to the user. Whether re-run would retry the WIF cleanup depends on forge client idempotency for absent-file deletion.

[pattern-violation] internal/repos/uninstall.go — goroutine/semaphore pattern divergence

Uninstall spawns goroutines before acquiring the semaphore (go func first, then select on sem inside the goroutine), while Sync, Diff, Upgrade, and Status acquire the semaphore in the for-loop before spawning. Both patterns are correct but the divergence creates O(n) goroutine stack space upfront in Uninstall vs at-most-maxConcurrency in the other functions.

[error-handling-idiom] internal/repos/uninstall.go — error message capitalization inconsistency

sync.go and upgrade.go use validateConcurrency which produces lowercase "concurrency must be between 1 and 32". uninstall.go uses inline validation with capitalized field name "MaxConcurrency must be between 1 and 32". The uninstall variant exposes the struct field name to users rather than using the shared validator or the CLI flag name.

Strengths

• Prior review's blocking compilation error (fakeProvisioner missing DeleteWIFProvider) fully addressed.
• PR body now explicitly calls out ADR 0057 edits in the "## ADR edits" section.
• CLI tree in docs/guides/dev/cli-internals.md now includes all new subcommands.
• filterRepos updated to use matchesPattern with filepath.Match for proper glob support across all repo-filtering commands.
• Comprehensive test coverage: 30+ upgrade tests, 40+ uninstall tests, 30+ sync/diff tests, 15+ manifest edit tests.
• Two-phase uninstall (parallel cleanup → sequential WIF) correctly handles the read-modify-write constraint on mint env vars.
• Glob matching via filepath.Match is safe against ReDoS and correctly integrated with confirmGlobAction for destructive operations.
• ProbeRepoState refactor eliminates code duplication between discoverRepo and the new commands.
• WIF Provider ID construction chain is well-defended: SplitN validation → ToLower → BuildRepoProviderID → url.PathEscape.
• Secrets handling properly separates non-sensitive variables (logged in cleartext) from sensitive secrets (never displayed).
• writeManifest file permissions (0o644) and repo name validation (repoNamePattern) are appropriate.
• Error handling follows established fmt.Errorf("verb: %w", err) pattern throughout.
• Manifest validation (Validate()) called in all new command paths.
• ProvisionerFactory type signature correctly updated from InstallConfig to ResolvedConfig.
• ADR 0057 Implementation Status section appropriately updated.
• No security findings: token handling follows established patterns, IsValidRef provides strict allowlist defense at three layers, $ escaping in replaceShimRef prevents regex backreference injection, forge abstraction respected throughout.
• Semver prerelease comparison correctly implements §11 with proper numeric vs string handling.

Previous run (6)

Review

Re-review of 4f06327 (prior review at b8d2cb7, provenance: app-verified).

The latest commit adds DeleteWIFProvider to fakeProvisioner in upgrade_test.go, resolving the prior review's blocking compilation error. The prior stale-doc findings (CLI tree missing upgrade/upgrade-mint, ProvisionerFactory type signature) are also resolved. One high-severity finding from the prior review remains unaddressed: the PR title is still missing the ! suffix for the breaking change.

High

[breaking-cli] PR title missing ! suffix for breaking change

Commit 1 is feat(repos)!: add, remove, install, uninstall subcommands — the ! indicates a breaking change (removes --repo flag from repos install, replaces with positional args). The PR title is feat(repos): add upgrade and upgrade-mint subcommands — missing !. Per COMMITS.md: "Breaking changes must carry the ! suffix in both commit messages and PR titles; a missing ! is an important-severity review finding." GoReleaser uses PR titles to build release notes, so users will not see the breaking change warning.

Remediation: Update the PR title to include the ! suffix, e.g. feat(repos)!: add repos management commands (add, remove, install, uninstall, diff, sync, upgrade, upgrade-mint).

Medium

[incomplete-destructive-ordering] internal/cli/repos.go — chained destructive operations in DeletePerRepoWIF

splitProjectAdapter.DeletePerRepoWIF chains two destructive operations: (1) mint.DeletePerRepoWIF (deregisters from mint) and (2) inference.DeleteWIFProvider (deletes GCP IAM WIF provider). If step 2 fails after step 1 succeeds, creates a partially-torn-down state with an orphaned WIF provider. The error message correctly notes "mint deregistration already succeeded — re-run is safe", which is good defensive messaging, but there is no separate tracking of which sub-step failed in the UninstallResult.

Low

[scope-creep] PR title and body do not reflect actual scope

The PR title says "add upgrade and upgrade-mint subcommands" and the body only describes those two commands, but the PR actually delivers 8 new/modified subcommands: add, remove, install (modified breaking change), uninstall, diff, sync, upgrade, upgrade-mint. The three commit messages correctly describe the full scope.

[adr-amendment-scope] docs/ADRs/0057-repos-management.md — modification not mentioned in PR description

ADR 0057 (Accepted on main) is modified to update the Implementation Status section. The changes are minor annotations ("is being implemented" → "is implemented", adding PR cross-references) which are allowed, but AGENTS.md asks to "call out any edits to accepted ADRs in the PR description."

[pattern-violation] internal/repos/uninstall.go — goroutine/semaphore pattern divergence

Uninstall spawns goroutines before acquiring the semaphore (go func first, then select on sem inside the goroutine), while Sync, Diff, Upgrade, and Status acquire the semaphore in the for-loop before spawning. Both patterns are correct but the divergence creates O(n) goroutine stack space upfront in Uninstall vs at-most-maxConcurrency in the other functions.

[error-handling-idiom] internal/repos/sync.go / internal/repos/upgrade.go — error message capitalization inconsistency

sync.go uses "concurrency must be between 1 and 32" (lowercase, matching Go convention) while upgrade.go uses "MaxConcurrency must be between 1 and 32" (capitalized field name).

[naming-convention] internal/cli/repos.go — Testing overrides comment slightly truncated

New config structs use a shorter form of the "Testing overrides" comment compared to the established pattern in admin.go which includes "Not set by CLI flag parsing."

Strengths

• Prior review's blocking compilation error (fakeProvisioner missing DeleteWIFProvider) fully addressed.
• CLI tree in docs/guides/dev/cli-internals.md now includes all new subcommands (add, remove, uninstall, diff, sync, upgrade, upgrade-mint).
• filterRepos updated to use matchesPattern with filepath.Match for proper glob support across all repo-filtering commands.
• Comprehensive test coverage: 30+ upgrade tests, 40+ uninstall tests, 30+ sync/diff tests, 15+ manifest edit tests.
• Two-phase uninstall (parallel cleanup → sequential WIF) correctly handles the read-modify-write constraint on mint env vars.
• Glob matching via filepath.Match is safe against ReDoS and correctly integrated with confirmGlobAction for destructive operations.
• ProbeRepoState refactor eliminates code duplication between discoverRepo and the new commands.
• WIF Provider ID construction chain is well-defended: SplitN validation → ToLower → BuildRepoProviderID → url.PathEscape.
• Secrets handling properly separates non-sensitive variables (logged in cleartext) from sensitive secrets (never displayed).
• writeManifest file permissions (0o644) and repo name validation (repoNamePattern) are appropriate.
• Error handling follows established fmt.Errorf("verb: %w", err) pattern throughout.
• Manifest validation (Validate()) called in all new command paths.
• ProvisionerFactory type signature correctly updated from InstallConfig to ResolvedConfig.
• ADR 0057 Implementation Status section appropriately updated.

Previous run (7)

Review

Re-review of 4f06327 (prior review at b8d2cb7, provenance: app-verified).

The latest commit adds DeleteWIFProvider to fakeProvisioner in upgrade_test.go, resolving the prior review's blocking compilation error. The prior stale-doc findings (CLI tree missing upgrade/upgrade-mint, ProvisionerFactory type signature) are also resolved. One high-severity finding from the prior review remains unaddressed: the PR title is still missing the ! suffix for the breaking change.

High

[breaking-cli] PR title missing ! suffix for breaking change

Commit 1 is feat(repos)!: add, remove, install, uninstall subcommands — the ! indicates a breaking change (removes --repo flag from repos install, replaces with positional args). The PR title is feat(repos): add upgrade and upgrade-mint subcommands — missing !. Per COMMITS.md: "Breaking changes must carry the ! suffix in both commit messages and PR titles; a missing ! is an important-severity review finding." GoReleaser uses PR titles to build release notes, so users will not see the breaking change warning.

Remediation: Update the PR title to include the ! suffix, e.g. feat(repos)!: add repos management commands (add, remove, install, uninstall, diff, sync, upgrade, upgrade-mint).

Medium

[incomplete-destructive-ordering] internal/cli/repos.go — chained destructive operations in DeletePerRepoWIF

splitProjectAdapter.DeletePerRepoWIF chains two destructive operations: (1) mint.DeletePerRepoWIF (deregisters from mint) and (2) inference.DeleteWIFProvider (deletes GCP IAM WIF provider). If step 2 fails after step 1 succeeds, creates a partially-torn-down state with an orphaned WIF provider. The error message correctly notes "mint deregistration already succeeded — re-run is safe", which is good defensive messaging, but there is no separate tracking of which sub-step failed in the UninstallResult.

Low

[scope-creep] PR title and body do not reflect actual scope

The PR title says "add upgrade and upgrade-mint subcommands" and the body only describes those two commands, but the PR actually delivers 8 new/modified subcommands: add, remove, install (modified breaking change), uninstall, diff, sync, upgrade, upgrade-mint. The three commit messages correctly describe the full scope.

[adr-amendment-scope] docs/ADRs/0057-repos-management.md — modification not mentioned in PR description

ADR 0057 (Accepted on main) is modified to update the Implementation Status section. The changes are minor annotations ("is being implemented" → "is implemented", adding PR cross-references) which are allowed, but AGENTS.md asks to "call out any edits to accepted ADRs in the PR description."

[pattern-violation] internal/repos/uninstall.go — goroutine/semaphore pattern divergence

Uninstall spawns goroutines before acquiring the semaphore (go func first, then select on sem inside the goroutine), while Sync, Diff, Upgrade, and Status acquire the semaphore in the for-loop before spawning. Both patterns are correct but the divergence creates O(n) goroutine stack space upfront in Uninstall vs at-most-maxConcurrency in the other functions.

[error-handling-idiom] internal/repos/sync.go / internal/repos/upgrade.go — error message capitalization inconsistency

sync.go uses "concurrency must be between 1 and 32" (lowercase, matching Go convention) while upgrade.go uses "MaxConcurrency must be between 1 and 32" (capitalized field name).

[naming-convention] internal/cli/repos.go — Testing overrides comment slightly truncated

New config structs use a shorter form of the "Testing overrides" comment compared to the established pattern in admin.go which includes "Not set by CLI flag parsing."

Strengths

• Prior review's blocking compilation error (fakeProvisioner missing DeleteWIFProvider) fully addressed.
• CLI tree in docs/guides/dev/cli-internals.md now includes all new subcommands (add, remove, uninstall, diff, sync, upgrade, upgrade-mint).
• filterRepos updated to use matchesPattern with filepath.Match for proper glob support across all repo-filtering commands.
• Comprehensive test coverage: 30+ upgrade tests, 40+ uninstall tests, 30+ sync/diff tests, 15+ manifest edit tests.
• Two-phase uninstall (parallel cleanup → sequential WIF) correctly handles the read-modify-write constraint on mint env vars.
• Glob matching via filepath.Match is safe against ReDoS and correctly integrated with confirmGlobAction for destructive operations.
• ProbeRepoState refactor eliminates code duplication between discoverRepo and the new commands.
• WIF Provider ID construction chain is well-defended: SplitN validation → ToLower → BuildRepoProviderID → url.PathEscape.
• Secrets handling properly separates non-sensitive variables (logged in cleartext) from sensitive secrets (never displayed).
• writeManifest file permissions (0o644) and repo name validation (repoNamePattern) are appropriate.
• Error handling follows established fmt.Errorf("verb: %w", err) pattern throughout.
• Manifest validation (Validate()) called in all new command paths.
• ProvisionerFactory type signature correctly updated from InstallConfig to ResolvedConfig.
• ADR 0057 Implementation Status section appropriately updated.

Previous run (8)

Review

Re-review of b8d2cb7 (prior review at 8d8ae12, provenance: app-verified).

The PR has been significantly expanded since the prior review: two new commits add repos add, repos remove, repos uninstall, repos diff, and repos sync commands on top of the previously-reviewed repos upgrade and repos upgrade-mint. The prior review's approval covered only the upgrade/upgrade-mint functionality — the new scope requires a fresh review.

High

[compilation-error] internal/repos/upgrade_test.go — fakeProvisioner missing DeleteWIFProvider

The fakeProvisioner struct in upgrade_test.go implements 5 of 6 WIFProvisioner methods but is missing DeleteWIFProvider(ctx context.Context, repo string) error. This PR adds DeleteWIFProvider to the WIFProvisioner interface in install.go. The test file will fail to compile: fakeProvisioner is used as WIFProvisioner in TestUpgradeMint_Success, TestUpgradeMint_URLMismatch, TestUpgradeMint_DiscoverError, TestUpgradeMint_EmptyURL, and TestUpgradeMint_ProgressCallback.

Every other fake provisioner in the PR (batchFakeProvisioner, perRepoProvisioner, cancellingOrgMintProvisioner, cancellingProvisioner, trackingOrgProvisioner, uninstallFakeProvisioner, sequentialUninstallProvisioner, trackingProvisioner, testWIFProvisioner, fakeWIFProvisioner) includes the stub — this one was missed.

Remediation: Add func (f *fakeProvisioner) DeleteWIFProvider(_ context.Context, _ string) error { return nil } to the struct.

[breaking-change-pr-title] PR title missing ! suffix for breaking change

Commit 1 is feat(repos)!: add, remove, install, uninstall subcommands — the ! indicates a breaking change (removes --repo flag from repos install, replaces with positional args). The PR title is feat(repos): add upgrade and upgrade-mint subcommands — missing !. Per AGENTS.md: "Breaking changes must carry the ! suffix in both commit messages and PR titles; a missing ! is an important-severity review finding." GoReleaser uses PR titles to build release notes, so users will not see the breaking change warning.

Remediation: Update the PR title to include the ! suffix and reflect the full scope, e.g. feat(repos)!: add repos management commands (add, remove, install, uninstall, diff, sync, upgrade, upgrade-mint).

Medium

[stale-doc] docs/guides/dev/cli-internals.md — CLI tree missing upgrade and upgrade-mint

The CLI command tree was updated to include add, remove, uninstall, diff, and sync under repos, but upgrade [repos...] and upgrade-mint are absent despite being registered as subcommands in this same PR. The tree terminates at sync with └──, implying no further children.

[scope-creep] PR title and body do not reflect actual scope

The PR title says "add upgrade and upgrade-mint subcommands" and the body only describes those two commands, but the PR actually delivers 8+ new or modified subcommands: add, remove, uninstall, diff, sync, upgrade, upgrade-mint, plus breaking changes to install (positional args replacing --repo flag). The three commit messages correctly describe the full scope, but the PR title and body are misleading.

Low

[adr-amendment-scope] docs/ADRs/0057-repos-management.md — modification not mentioned in PR description

ADR 0057 (Accepted on main) is modified to update the Implementation Status section. The changes are minor annotations ("is being implemented" → "is implemented", adding PR cross-references) which are allowed, but AGENTS.md asks to "call out any edits to accepted ADRs in the PR description."

[pattern-violation] internal/repos/uninstall.go — goroutine/semaphore pattern divergence

Uninstall spawns goroutines before acquiring the semaphore (go func first, then select on sem inside the goroutine), while Sync and Upgrade acquire the semaphore in the for-loop before spawning. Both patterns are correct but the divergence may surprise future maintainers.

[stale-doc] docs/plans/repos-management.md — ProvisionerFactory type signature

The plan document shows type ProvisionerFactory func(cfg InstallConfig) WIFProvisioner but the actual implementation uses ResolvedConfig.

Strengths

• All prior review findings from upgrade/upgrade-mint (across 11 iterations) fully addressed.
• Comprehensive test coverage: 30+ upgrade tests, 40+ uninstall tests, 30+ sync/diff tests, 15+ manifest edit tests.
• Two-phase uninstall (parallel cleanup → sequential WIF) correctly handles the read-modify-write constraint on mint env vars.
• Glob matching via filepath.Match is safe against ReDoS and correctly integrated with confirmGlobAction for destructive operations.
• ProbeRepoState refactor eliminates code duplication between discoverRepo and checkRepoStatus.
• Manifest ref validation at parse time (Validate) provides defense-in-depth alongside per-repo upgradeRepo() checks.
• Secrets handling properly separates non-sensitive variables (logged in cleartext) from sensitive secrets (never displayed).
• writeManifest file permissions (0o644) and repo name validation (repoNamePattern) are appropriate.
• Error handling follows established fmt.Errorf("verb: %w", err) pattern throughout.

Previous run (9)

Review

Re-review of b8d2cb7 (prior review at 8d8ae12, provenance: app-verified).

The PR has been significantly expanded since the prior review: two new commits add repos add, repos remove, repos uninstall, repos diff, and repos sync commands on top of the previously-reviewed repos upgrade and repos upgrade-mint. The prior review's approval covered only the upgrade/upgrade-mint functionality — the new scope requires a fresh review.

High

[compilation-error] internal/repos/upgrade_test.go — fakeProvisioner missing DeleteWIFProvider

The fakeProvisioner struct in upgrade_test.go implements 5 of 6 WIFProvisioner methods but is missing DeleteWIFProvider(ctx context.Context, repo string) error. This PR adds DeleteWIFProvider to the WIFProvisioner interface in install.go. The test file will fail to compile: fakeProvisioner is used as WIFProvisioner in TestUpgradeMint_Success, TestUpgradeMint_URLMismatch, TestUpgradeMint_DiscoverError, TestUpgradeMint_EmptyURL, and TestUpgradeMint_ProgressCallback.

Every other fake provisioner in the PR (batchFakeProvisioner, perRepoProvisioner, cancellingOrgMintProvisioner, cancellingProvisioner, trackingOrgProvisioner, uninstallFakeProvisioner, sequentialUninstallProvisioner, trackingProvisioner, testWIFProvisioner, fakeWIFProvisioner) includes the stub — this one was missed.

Remediation: Add func (f *fakeProvisioner) DeleteWIFProvider(_ context.Context, _ string) error { return nil } to the struct.

[breaking-change-pr-title] PR title missing ! suffix for breaking change

Commit 1 is feat(repos)!: add, remove, install, uninstall subcommands — the ! indicates a breaking change (removes --repo flag from repos install, replaces with positional args). The PR title is feat(repos): add upgrade and upgrade-mint subcommands — missing !. Per AGENTS.md: "Breaking changes must carry the ! suffix in both commit messages and PR titles; a missing ! is an important-severity review finding." GoReleaser uses PR titles to build release notes, so users will not see the breaking change warning.

Remediation: Update the PR title to include the ! suffix and reflect the full scope, e.g. feat(repos)!: add repos management commands (add, remove, install, uninstall, diff, sync, upgrade, upgrade-mint).

Medium

[stale-doc] docs/guides/dev/cli-internals.md — CLI tree missing upgrade and upgrade-mint

The CLI command tree was updated to include add, remove, uninstall, diff, and sync under repos, but upgrade [repos...] and upgrade-mint are absent despite being registered as subcommands in this same PR. The tree terminates at sync with └──, implying no further children.

[scope-creep] PR title and body do not reflect actual scope

The PR title says "add upgrade and upgrade-mint subcommands" and the body only describes those two commands, but the PR actually delivers 8+ new or modified subcommands: add, remove, uninstall, diff, sync, upgrade, upgrade-mint, plus breaking changes to install (positional args replacing --repo flag). The three commit messages correctly describe the full scope, but the PR title and body are misleading.

Low

[adr-amendment-scope] docs/ADRs/0057-repos-management.md — modification not mentioned in PR description

ADR 0057 (Accepted on main) is modified to update the Implementation Status section. The changes are minor annotations ("is being implemented" → "is implemented", adding PR cross-references) which are allowed, but AGENTS.md asks to "call out any edits to accepted ADRs in the PR description."

[pattern-violation] internal/repos/uninstall.go — goroutine/semaphore pattern divergence

Uninstall spawns goroutines before acquiring the semaphore (go func first, then select on sem inside the goroutine), while Sync and Upgrade acquire the semaphore in the for-loop before spawning. Both patterns are correct but the divergence may surprise future maintainers.

[stale-doc] docs/plans/repos-management.md — ProvisionerFactory type signature

The plan document shows type ProvisionerFactory func(cfg InstallConfig) WIFProvisioner but the actual implementation uses ResolvedConfig.

Strengths

• All prior review findings from upgrade/upgrade-mint (across 11 iterations) fully addressed.
• Comprehensive test coverage: 30+ upgrade tests, 40+ uninstall tests, 30+ sync/diff tests, 15+ manifest edit tests.
• Two-phase uninstall (parallel cleanup → sequential WIF) correctly handles the read-modify-write constraint on mint env vars.
• Glob matching via filepath.Match is safe against ReDoS and correctly integrated with confirmGlobAction for destructive operations.
• ProbeRepoState refactor eliminates code duplication between discoverRepo and checkRepoStatus.
• Manifest ref validation at parse time (Validate) provides defense-in-depth alongside per-repo upgradeRepo() checks.
• Secrets handling properly separates non-sensitive variables (logged in cleartext) from sensitive secrets (never displayed).
• writeManifest file permissions (0o644) and repo name validation (repoNamePattern) are appropriate.
• Error handling follows established fmt.Errorf("verb: %w", err) pattern throughout.

Previous run (10)

Review

Re-review of 8d8ae12 (prior review at 4f5ca56, provenance: app-verified).

All prior findings have been addressed or resolved:

• ✅ extractWorkflowRef/shimRefPattern scope mismatch (medium) — The prior review claimed the upgrade would be silently skipped when the workflow ref matches the target but a composite-action ref is stale, citing a currentRef == targetRef early return at upgrade.go:165. This was incorrect: the code has no such early return. The semver comparison only skips when compareSemver(currentRef, targetRef) > 0 (current is newer), not when they are equal. When the workflow ref matches the target but an action ref is stale, replaceShimRef correctly detects the difference via shimRefPattern (which matches all fullsend-ai/fullsend/ uses: lines) and proceeds with the upgrade. TestUpgrade_MixedRefsWorkflowAtTargetActionStale explicitly validates this scenario.
• ✅ parseUint has no uint64 overflow guard (low) — parseUint now includes a maxSafe guard (^uint64(0)/10 - 1) that returns ^uint64(0) on overflow, preventing silent wrap-around.
• ✅ containsRef test helper has redundant conditions (low) — The helper has been simplified to findRefInContent with clearer line-splitting logic.
• i Positional args vs --repo flag inconsistency (low) — Remains a deliberate design choice, confirmed by the test at line 456 asserting --repo should not exist on upgrade. This is an API-shape observation, not a defect.

Strengths

• All prior review findings (across 11 iterations, from high through low) fully addressed.
• Manifest validation now validates fullsend_ref at parse time via IsValidRef for both defaults.fullsend_ref and per-repo overrides, providing defense-in-depth alongside the per-repo upgradeRepo() check.
• Semver prerelease comparison correctly implements §11 with comparePrerelease() and comparePreID() — dot-separated, numeric segments as integers, numeric < string.
• Build metadata correctly excluded from prerelease comparison per semver 2.0.0 §10 via (?:-([^+]+))?.
• parseUint overflow guard prevents silent wrap-around on crafted version numbers.
• $ escaping in replaceShimRef prevents regex backreference injection.
• shimRefPattern uses (\s*#.*)? to correctly handle multi-word trailing comments.
• Ref validation (IsValidRef) at three layers: CLI flag, manifest validation, per-repo execution.
• Mixed-ref scenario (workflow at target, action stale) is correctly handled and tested.
• Concurrency pattern (semaphore + WaitGroup) consistent with Status() and BatchInstall().
• MaxConcurrency validation returns an error (matching BatchInstall() pattern), not a silent default.
• Error handling follows established fmt.Errorf("verb: %w", err) pattern.
• checkPerRepoScopes preflight properly included, matching runReposInstall.
• repos upgrade-mint is verification-only (read-only DiscoverMint), appropriate security posture.
• Documentation for both commands present in docs/cli/repos.md and docs/guides/getting-started/operations.md.
• ADR 0057 Implementation Status section appropriately updated (minor annotation, not substantive rewrite).
• Implementation plan correctly updated with completion marker, flag list changes, and scope annotations.
• PR title feat(repos): correctly follows COMMITS.md for new user-facing CLI commands (not a breaking change).
• No security findings: token handling follows established patterns, commit messages use static strings, forge abstraction respected.
• Code follows all established codebase conventions (config struct naming, // Testing overrides comments, error handling idioms, test organization).
• Test coverage comprehensive: 30+ upgrade tests covering edge cases, table-driven helpers, direct flag threading, mixed-ref scenarios, and prerelease handling.

Previous run (11)

Review

Re-review of 4f5ca56 (prior review at 5c06536, provenance: app-verified).

Both prior low-severity findings have been addressed:

• ✅ Plan test strategy references removed --repo flag — line 1146 now reads Positional args filter, matching the implementation.
• ✅ Plan flag list omits --direct — --direct is now included in the plan's flag list at line 1051.

Medium

extractWorkflowRef/shimRefPattern scope mismatch (medium)

extractWorkflowRef (from status.go) only matches .github/workflows/ paths, while shimRefPattern in upgrade.go matches all fullsend-ai/fullsend/ paths (including .github/actions/). When the main workflow uses: line is already at the target ref but a composite-action uses: line is at an older ref, the upgrade is skipped because currentRef == targetRef passes at internal/repos/upgrade.go:165, and replaceShimRef — which would catch the stale action refs — is never reached.

The scaffold templates embed fullsend-ai/fullsend/.github/actions/mint-token@__FULLSEND_AI_REF__ alongside the workflow uses: line. If a partial upgrade left the workflow ref updated but the action ref stale, subsequent runs would silently skip the repo.

Remediation: Either widen extractWorkflowRef to match any fullsend-ai/fullsend uses: line, or remove the currentRef == targetRef early return and rely on replaceShimRef's changed boolean as the sole skip signal.

Low

parseUint has no uint64 overflow guard (low)

parseUint in internal/repos/upgrade.go performs no overflow check. A crafted ref like v99999999999999999999.0.0 would silently wrap, causing compareSemver to return incorrect ordering. IsValidRef constrains the character set but not length. Practical risk is near-zero since refs come from manifest YAML and workflow files.

containsRef test helper has redundant conditions (low)

containsRef in internal/repos/upgrade_test.go includes conditions that are always true for its call sites (e.g., fmt.Sprintf("@%s", ref) != ""). The function returns correct results but the redundant guards could mask a test bug if the underlying pattern stopped matching.

Positional args vs --repo flag inconsistency (low)

repos upgrade uses positional args ([repos...]) for repo filtering while repos status uses a --repo repeatable flag. The test at line 456 explicitly asserts --repo should not exist on upgrade, confirming this was a deliberate design choice. The inconsistency within the repos command family is a minor API-shape concern.

Strengths

• All prior review findings (across 10 iterations, from high through low) fully addressed.
• Manifest validation now provides defense-in-depth for both CLI-sourced and manifest-sourced refs.
• Semver prerelease comparison correctly implements §11 with comparePrerelease() and comparePreID().
• Build metadata correctly excluded from prerelease comparison per semver 2.0.0 §10.
• $ escaping in replaceShimRef prevents regex backreference injection.
• shimRefPattern uses (\s*#.*)? to correctly handle multi-word trailing comments.
• Ref validation (IsValidRef) at three layers: CLI flag, manifest validation, per-repo execution.
• Concurrency pattern (semaphore + WaitGroup) consistent with Status() and BatchInstall().
• Error handling follows established fmt.Errorf("verb: %w", err) pattern.
• checkPerRepoScopes preflight properly included, matching runReposInstall.
• repos upgrade-mint is verification-only (read-only DiscoverMint), appropriate security posture.
• Documentation for both commands present in docs/cli/repos.md and docs/guides/getting-started/operations.md.
• ADR 0057 Implementation Status section appropriately updated (minor annotation, not substantive rewrite).
• No security findings: token handling follows established patterns, commit messages use static strings, forge abstraction respected.
• Code follows all established codebase conventions (config struct naming, error handling idioms, test organization).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:00 AM UTC · Completed 2:13 AM UTC
Commit: 2c0f558 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:24 AM UTC · Completed 2:38 AM UTC
Commit: 368bdf2 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 2:51 AM UTC · Completed 3:03 AM UTC
Commit: eed56eb · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:07 AM UTC · Completed 3:23 AM UTC
Commit: 6e2493a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:27 AM UTC · Completed 3:41 AM UTC
Commit: 7262ac4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:45 AM UTC · Completed 3:56 AM UTC
Commit: c26c56a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:25 PM UTC · Completed 6:41 PM UTC
Commit: 6e05ef7 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:52 PM UTC · Completed 7:05 PM UTC
Commit: 15207a0 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 7:27 PM UTC · Completed 7:42 PM UTC
Commit: 15207a0 · View workflow run →

[fullsend-ai-retro]

Retrospective analysis of PR #4080 (feat(repos): add upgrade and upgrade-mint subcommands), a stacked PR implementing fullsend repos upgrade and fullsend repos upgrade-mint. The PR went through 9+ review iterations over 7 days with 63 dispatch runs, 23 force-pushes, and 13 review bot submissions before human approval and merge.

Key findings:

1. Stacked PR handling was the docs: Add agent-compatible code problem document #1 review quality gap. The review agent reviewed the full PR diff (base branch to HEAD), including code from PR feat(repos)!: add repos diff and repos sync CLI commands #4079 in the stack, producing 5 out-of-scope findings and a persistent false-positive HIGH finding about the PR title missing ! (repeated 7+ times despite author explanation). This is additional evidence for existing issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #4680.

2. Severity miscalibration on a HIGH-impact regex bug. The bot correctly identified the \s* regex matching newlines in replaceShimRef but rated it Low/"benign." The human reviewer demonstrated it silently deletes standalone YAML comment lines (data loss), correctly rating it HIGH. The bot lacks a mechanism to construct concrete reproductions before assigning severity.

3. Human reviewer caught all 5 high-impact findings. The bot missed partial-version-tag bypass, SHA-pin rewriting, glob documentation gap, regex comment-line deletion (at correct severity), and plan/implementation divergence. The bot's unique findings were all low-severity edge cases. This is counter-evidence for review autonomy on complex Go code with regex/semver logic.

4. Author explanations not carried into re-review anchoring. The ! suffix finding was repeated across every iteration because the re-review mechanism anchors severity but does not incorporate the author's response context explaining why the finding doesn't apply to this PR.

Proposals filed: 3 improvement proposals targeting fullsend-ai/fullsend.

Proposals filed

• Review agent correctness sub-agent should construct concrete impact examples before assigning severity to edge-case findings (in fullsend-ai/fullsend)
• Review re-review anchoring should incorporate author response context to suppress explained-away findings (in fullsend-ai/fullsend)
• Counter-evidence for review autonomy: human reviewer caught all high-impact findings on complex Go regex/semver PR #4080 (in fullsend-ai/fullsend)