feat(repos)!: add repos diff and repos sync CLI commands

[ggallen]

Summary

• Add fullsend repos diff command — read-only preview of configuration drift between the repos.yaml manifest and actual forge state
• Add fullsend repos sync command — writes variables and secrets to reconcile repos with the manifest
• Sync reconciles 3 variables (FULLSEND_PER_REPO_INSTALL, FULLSEND_MINT_URL, FULLSEND_GCP_REGION) and 1 secret (FULLSEND_GCP_PROJECT_ID); does not touch scaffold shim version or harness files (managed by repos upgrade)
• Guard variable (FULLSEND_PER_REPO_INSTALL) is reconciled per the plan spec — repos with a missing or incorrect guard will report drift and be healed by sync
• checkPerRepoScopes preflight runs for both diff and sync, with --json-safe output (printer writes to io.Discard in JSON mode)
• CLI wiring follows the config-struct + run*() pattern from feat(repos)!: add, remove, install, uninstall subcommands #4081 with testClient DI for integration testing

Implements PR 6 from the repos management plan.

Files changed

File	Description
internal/repos/sync.go	Core Diff(), Sync(), FormatDiffTable() with semaphore-bounded concurrency
internal/repos/sync_test.go	38 tests — 89-100% coverage across all sync.go functions
internal/cli/repos.go	reposDiffConfig/reposSyncConfig structs, runReposDiff()/runReposSync() functions, cobra command wiring
internal/cli/repos_test.go	Flag tests + 6 integration tests calling run*() directly with fake clients

Test plan

• go test ./internal/repos/ — all 38 sync tests pass
• go test ./internal/cli/ — all repos CLI tests pass (flag tests + integration tests)
• go build ./... — clean build
• Coverage for sync.go: Diff 93.3%, diffRepo 100%, Sync 89.7%, ensureSecrets/applyChanges/validateConcurrency/FormatDiffTable 100%
• CI checks

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:36 PM UTC · Completed 9:45 PM UTC
Commit: 1dcb8e3 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

feat(repos): add diff/sync commands to reconcile repos.yaml drift

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add fullsend repos diff to preview manifest vs forge configuration drift.
• Add fullsend repos sync to apply variables/secrets to match repos.yaml.
• Provide JSON/table output, repo filtering, dry-run, and bounded concurrency.

Diagram

graph TD
  U["Operator"] --> CLI["fullsend repos diff/sync"] --> Sync["internal/repos/sync.go"] --> Forge["forge.Client"] --> GH{{"GitHub API"}}
  CLI --> Manifest["repos.yaml manifest"] --> Sync

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Only create missing secrets (treat existing as in-sync)

• ➕ repos diff reflects true drift instead of always showing secret updates
• ➕ Avoids repeated secret write operations and related audit noise
• ➕ Keeps exit-code semantics aligned with 'drift exists'
• ➖ Cannot guarantee secret value matches manifest when it changes
• ➖ May require an explicit separate pathway to rotate/update secrets

2. Add explicit secret behavior flags (e.g., --sync-secrets/--force-secrets)

• ➕ Preserves secure default of not assuming secret correctness
• ➕ Gives operators control over whether diff/sync includes secret updates
• ➕ Enables CI-friendly drift checks without constant secret churn
• ➖ More CLI surface area and documentation burden
• ➖ More branching in implementation and tests

3. Use a secret 'fingerprint' variable to detect intended secret changes

• ➕ Allows diff to be accurate without reading secret values
• ➕ Sync can update secrets only when fingerprint changes
• ➖ Adds another managed field and rollout complexity
• ➖ Requires agreement on fingerprint computation and rotation process

Recommendation: The current approach is solid for variables and for ensuring secret presence, but the chosen semantics "secrets are always re-synced" means repos diff will frequently report changes even when variables match, and its exit-code-1 drift signal may become noisy. Consider either (a) only creating missing secrets by default, or (b) adding an explicit flag to include secret updates, so diff remains a reliable drift detector while still supporting intentional secret rotation.

Files changed (4) +1434 / -0

Enhancement (2) +494 / -0

repos.goAdd repos diff/sync cobra subcommands and flags +146/-0

Add repos diff/sync cobra subcommands and flags

• Registers new 'diff' and 'sync' subcommands under 'fullsend repos'. Implements manifest loading/validation, repo filtering, JSON vs table output, concurrency control, and '--dry-run' behavior for sync.

internal/cli/repos.go

sync.goImplement manifest drift Diff and reconciliation Sync +348/-0

Implement manifest drift Diff and reconciliation Sync

• Adds a diff engine that compares desired manifest-derived config to actual repo variables/secrets for installed repos (guarded by 'FULLSEND_PER_REPO_INSTALL=true'). Adds a sync engine that groups changes per repo (sequential within repo) while applying across repos with semaphore-bounded concurrency, plus table formatting for human output.

internal/repos/sync.go

Tests (2) +940 / -0

repos_test.goTest diff/sync subcommand registration and flags +56/-0

Test diff/sync subcommand registration and flags

• Extends CLI tests to assert 'diff'/'sync' subcommands exist and validates defaults for '--manifest/-f', '--repo', '--json', '--concurrency', and '--dry-run'.

internal/cli/repos_test.go

sync_test.goAdd comprehensive unit tests for Diff/Sync and formatting +884/-0

Add comprehensive unit tests for Diff/Sync and formatting

• Introduces extensive tests using 'forge.NewFakeClient()' to cover drift detection, repo filtering, glob expansion, concurrency defaults, error-to-warning behavior, sync application for variables/secrets, and table rendering edge cases.

internal/repos/sync_test.go

[codecov]

Codecov Report

❌ Patch coverage is 79.25072% with 72 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/repos.go	65.07%	26 Missing and 18 partials ⚠️
internal/repos/sync.go	88.12%	20 Missing and 6 partials ⚠️
internal/repos/init.go	0.00%	1 Missing and 1 partial ⚠️

📢 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. Sync masks apply failures ✓ Resolved 🐞 Bug ☼ Reliability

Description

Sync() converts per-repo apply errors into Warnings and still returns a nil error, so the CLI exits
successfully even when some repos failed to update. This can silently leave repos unreconciled in
automation.

Code

internal/repos/sync.go[R258-268]

+	var allApplied []Change
+	var allWarnings []string
+	allWarnings = append(allWarnings, diffResult.Warnings...)
+	for _, r := range results {
+		allApplied = append(allApplied, r.applied...)
+		if r.err != nil {
+			allWarnings = append(allWarnings, r.err.Error())
+		}
+	}
+
+	return &SyncResult{Applied: allApplied, Warnings: allWarnings}, nil

Relevance

⭐⭐⭐ High

Fail-closed patterns preferred; prior accepted changes avoid masking unknown/failed operations as
success.

PR-#967
PR-#1982

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Sync stores apply errors but only appends them into Warnings and returns nil error; the CLI prints
warnings (only in non-JSON mode) and then returns nil, so the process exits 0 even on failed writes.
This differs from other repo operations that fail closed on critical errors (e.g., install guard
check).

internal/repos/sync.go[240-268]
internal/cli/repos.go[608-627]
internal/repos/install.go[171-182]

Agent prompt

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

### Issue description
`Sync()` aggregates per-repo apply errors into `SyncResult.Warnings` and returns `nil` error, and the CLI returns success regardless of warnings. This makes it hard (or impossible) for automation/CI to detect partial failures.

### Issue Context
The code already distinguishes success/failure per repo (`applyErr`) but does not propagate failure as an error/exit code.

### Fix Focus Areas
- internal/repos/sync.go[223-269]
- internal/cli/repos.go[608-627]

### Suggested fix
- Track `failed` repos in `Sync()` (or in the CLI) when `applyErr != nil`.
- After printing output, return a non-nil error (and set `cmd.SilenceUsage = true`) when any failures occurred.
 - Option A: make `Sync()` return an error when any repo failed.
 - Option B: keep `Sync()` returning results, but have CLI decide: if `len(result.Warnings) > 0` (or a dedicated `Failed` field), exit non-zero.
- Keep warnings in output, but ensure exit status reflects partial failure.

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

---

2. resolveToken() shells out to gh ✗ Dismissed 📘 Rule violation ⌂ Architecture

Description

The new repos diff and repos sync commands call resolveToken(), which executes
exec.Command("gh", ...) outside internal/forge/github/. This violates the restriction on gh
CLI subprocess usage and increases the blast radius of a disallowed mechanism.

Code

internal/cli/repos.go[R510-575]

+			token, err := resolveToken()
+			if err != nil {
+				return err
+			}
+			client := newGitHubLiveClient(token)
+
+			m, err := repos.LoadManifest(ctx, manifest)
+			if err != nil {
+				return err
+			}
+			if err := m.Validate(); err != nil {
+				return fmt.Errorf("manifest validation failed: %w", err)
+			}
+
+			result, err := repos.Diff(ctx, m, client, concurrency, repoFilter)
+			if err != nil {
+				return err
+			}
+
+			if jsonOutput {
+				b, marshalErr := json.MarshalIndent(result, "", "  ")
+				if marshalErr != nil {
+					return fmt.Errorf("marshalling JSON: %w", marshalErr)
+				}
+				fmt.Fprintln(cmd.OutOrStdout(), string(b))
+			} else {
+				fmt.Fprint(cmd.OutOrStdout(), repos.FormatDiffTable(result))
+			}
+
+			if len(result.Changes) > 0 {
+				cmd.SilenceUsage = true
+				return fmt.Errorf("%d changes needed to reconcile manifest", len(result.Changes))
+			}
+			return nil
+		},
+	}
+
+	cmd.Flags().StringVarP(&manifest, "manifest", "f", "repos.yaml", "path or HTTPS URL to manifest file")
+	cmd.Flags().StringArrayVar(&repoFilter, "repo", nil, "filter to specific repos (repeatable)")
+	cmd.Flags().BoolVar(&jsonOutput, "json", false, "emit JSON output instead of table")
+	cmd.Flags().IntVar(&concurrency, "concurrency", 4, "max parallel API calls")
+
+	return cmd
+}
+
+func newReposSyncCmd() *cobra.Command {
+	var (
+		manifest    string
+		repoFilter  []string
+		dryRun      bool
+		jsonOutput  bool
+		concurrency int
+	)
+
+	cmd := &cobra.Command{
+		Use:   "sync",
+		Short: "Reconcile configuration drift for installed repos",
+		Long:  "Apply variable and secret changes to reconcile installed repos with the manifest. Use --dry-run to preview changes without applying them.",
+		RunE: func(cmd *cobra.Command, args []string) error {
+			ctx := cmd.Context()
+
+			token, err := resolveToken()
+			if err != nil {
+				return err
+			}
+			client := newGitHubLiveClient(token)

Relevance

⭐⭐⭐ High

Team accepts strict GitHub token/gh boundary hardening; similar security restrictions merged
previously.

PR-#286
PR-#337

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance rule forbids gh CLI subprocess execution outside internal/forge/github/. The
added commands invoke resolveToken() (new call sites), and resolveToken() runs
exec.Command("gh", "auth", "token") in internal/cli/admin.go, which is not under
internal/forge/github/.

Rule 1062053: Restrict gh CLI exec.Command usage to internal/forge/github
internal/cli/repos.go[510-575]
internal/cli/admin.go[71-86]

Agent prompt

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

## Issue description
`internal/cli/repos.go` now calls `resolveToken()`, which shells out to `gh` via `exec.Command("gh", ...)`. Per compliance, `gh` CLI subprocess usage must be restricted to `internal/forge/github/`.

## Issue Context
The new `repos diff` and `repos sync` commands introduced additional call sites to `resolveToken()`.

## Fix Focus Areas
- internal/cli/repos.go[510-575]
- internal/cli/admin.go[71-86]

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

---

3. Sync JSON output polluted ✓ Resolved 🐞 Bug ≡ Correctness

Description

newReposSyncCmd() always prints progress updates to os.Stdout, even when --json is set, so stdout
contains both progress lines and JSON and is not machine-parseable. This breaks automation that
relies on fullsend repos sync --json.

Code

internal/cli/repos.go[R603-618]

+			printer := ui.New(os.Stdout)
+			progressFn := func(repo, phase, message string) {
+				printer.StepInfo(fmt.Sprintf("[%s] %s: %s", phase, repo, message))
+			}
+
+			result, err := repos.Sync(ctx, m, client, concurrency, repoFilter, progressFn)
+			if err != nil {
+				return err
+			}
+
+			if jsonOutput {
+				b, marshalErr := json.MarshalIndent(result, "", "  ")
+				if marshalErr != nil {
+					return fmt.Errorf("marshalling JSON: %w", marshalErr)
+				}
+				fmt.Fprintln(cmd.OutOrStdout(), string(b))

Relevance

⭐⭐⭐ High

Team has accepted CLI output/progress correctness fixes; polluted JSON output likely treated as
breaking automation.

PR-#2015
PR-#1682

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The sync command constructs a UI printer bound to stdout and emits StepInfo progress lines, while
also emitting JSON to cmd.OutOrStdout() (stdout by default). ui.Printer writes to its configured
writer, so progress and JSON will interleave.

internal/cli/repos.go[603-618]
internal/ui/ui.go[22-32]
internal/ui/ui.go[84-90]

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 sync` writes progress messages to `os.Stdout` via `ui.Printer` regardless of `--json`. When `--json` is enabled, the command later prints JSON to `cmd.OutOrStdout()` (typically stdout), resulting in invalid JSON output.

### Issue Context
`ui.Printer` writes directly to the `io.Writer` it is constructed with.

### Fix Focus Areas
- internal/cli/repos.go[603-624]
- internal/ui/ui.go[22-32]
- internal/ui/ui.go[84-90]

### Suggested fix
- When `jsonOutput` is true, either:
 1. Disable progress output entirely (pass `nil`/no-op progress func), **or**
 2. Route progress output to `cmd.ErrOrStderr()` by constructing the printer with `ui.New(cmd.ErrOrStderr())`.
- Ensure the stdout stream contains *only* JSON when `--json` is requested.

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

---

View more (1)

4. Glob overrides ignored in sync ✓ Resolved 🐞 Bug ≡ Correctness

Description

Sync() resolves repo config using Manifest.ResolveConfig(), which does not apply glob-entry
overrides, then uses that config to derive secret values, so glob-expanded repos can get the wrong
secret (including an empty value). Diff() correctly uses ResolveConfigForEntry(), so sync can
diverge from what diff computed as desired.

Code

internal/repos/sync.go[R244-247]

+			repoFullName := k.owner + "/" + k.repo
+			cfg, _ := manifest.ResolveConfig(k.owner, k.repo)
+
+			applied, applyErr := applyChanges(ctx, client, k.owner, k.repo, cfg, changes, progress)

Relevance

⭐⭐ Medium

No historical evidence found on glob override config resolution patterns in repos sync.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Manifest.ResolveConfig() is documented to not match glob patterns and to require
ResolveConfigForEntry() for glob-expanded repos; Diff() uses ResolveConfigForEntry(), but Sync()
uses ResolveConfig() and then applyChanges() derives secret values from that config, so glob
overrides can be dropped during secret writes.

internal/repos/sync.go[90-96]
internal/repos/sync.go[240-248]
internal/repos/sync.go[284-288]
internal/repos/manifest.go[487-522]

Agent prompt

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

### Issue description
`Sync()` uses `manifest.ResolveConfig(owner, repo)` when applying changes. This does **not** apply per-entry overrides for repos that came from glob expansion (e.g. `org/*`). Secret writes in `applyChanges()` derive the secret value from this resolved config, so sync can write an incorrect/empty secret value for glob-matched repos.

### Issue Context
- `Diff()` resolves config with `ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry)`.
- `Manifest.ResolveConfig()` explicitly does not match glob patterns.
- `applyChanges()` resolves secret values from the config, not from the `Change` payload.

### Fix Focus Areas
- internal/repos/sync.go[190-255]
- internal/repos/sync.go[271-304]
- internal/repos/manifest.go[487-522]

### Suggested fix
1. In `Sync()`, build a `map[repoKey]ResolvedConfig` by calling `manifest.ExpandGlobs(ctx, client)` and then `ResolveConfigForEntry(owner, repo, entry)` for each resolved repo (apply the same `repoFilter` logic as Diff).
2. When applying changes per repo, fetch config from that map (entry-aware), not `ResolveConfig()`.
3. Add a safety check for secrets: if the resolved secret value is empty, return an error for that repo rather than writing an empty secret.

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

---

5. Diff warnings hidden in table ✓ Resolved 🐞 Bug ◔ Observability

Description

Diff() can return warnings for API failures, but FormatDiffTable() ignores DiffResult.Warnings and
can print “No changes needed” even when some repos were not checked successfully. Both repos diff
and repos sync --dry-run table output paths therefore hide partial failures.

Code

internal/repos/sync.go[R306-310]

+// FormatDiffTable renders a DiffResult as a human-readable table.
+func FormatDiffTable(result *DiffResult) string {
+	if len(result.Changes) == 0 {
+		return "No changes needed — all repos match the manifest.\n"
+	}

Relevance

⭐⭐ Medium

No historical evidence found requiring table formatter to always surface warnings when no changes.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

diffRepo returns warnings instead of an error for some API failures; Diff aggregates these warnings,
but FormatDiffTable’s output ignores them and can print a clean “No changes needed” message based
solely on Changes. The CLI table paths for diff/dry-run sync only print FormatDiffTable output.

internal/repos/sync.go[111-116]
internal/repos/sync.go[306-310]
internal/cli/repos.go[529-537]
internal/cli/repos.go[585-600]

Agent prompt

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

### Issue description
`Diff()` returns `DiffResult.Warnings`, but the default human-readable output path uses `FormatDiffTable(result)` which does not display warnings. If there are warnings and zero changes, the table prints “No changes needed — all repos match the manifest.” which is misleading.

### Issue Context
Warnings are produced when API calls fail (e.g., listing variables / checking secrets) but the operation continues.

### Fix Focus Areas
- internal/repos/sync.go[111-116]
- internal/repos/sync.go[306-341]
- internal/cli/repos.go[529-537]
- internal/cli/repos.go[585-600]

### Suggested fix
- Update `FormatDiffTable()` to append a warnings section when `len(result.Warnings) > 0`, or
- Have the CLI print warnings to `cmd.ErrOrStderr()` after printing the table.
- Consider making warnings affect exit status (non-zero) so partial failures can’t be mistaken for a clean diff.

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

---

[fullsend-ai-review]

Review

Re-review after branch update (prior review SHA 1bea3a97, now 3c6424). All prior low findings re-evaluated against the updated code. The implementation is well-structured: config-struct + run*() pattern, forge abstraction respected, checkPerRepoScopes gates all commands including the newly-added uninstall preflight, secret values never appear in output, concurrency validation is strict (1–32), and test coverage is comprehensive (38 tests in sync_test.go, 9 integration tests in repos_test.go). Security analysis confirms correct fail-closed behavior at all auth/validation gates and proper secrets handling throughout.

Findings

Low

• [overly-restrictive-preflight] internal/cli/repos.go — Both runReposDiff and runReposSync call checkPerRepoScopes, which requires both repo and workflow OAuth scopes (via perRepoRequiredScopes). Neither command touches workflow files: repos diff only calls ListRepoVariables and RepoSecretExists, while repos sync only calls those plus CreateOrUpdateRepoVariable and CreateRepoSecret. The workflow scope is needed for managing .github/workflows/ files (used by repos install, repos uninstall, and repos upgrade) but is unnecessary here. Users with a PAT that has repo scope but not workflow will be incorrectly blocked.

• [information-exposure] internal/repos/sync.go — In applyChanges(), the progress callback logs variable values in cleartext via fmt.Sprintf("%s %s=%s", c.Action, c.Field, c.NewValue). Currently safe because only non-sensitive variables (FULLSEND_MINT_URL, FULLSEND_GCP_REGION) pass through this path, and the code comment on managedVariables explicitly designates it as the non-sensitive list with managedSecrets for sensitive data. Well-designed control that relies on future developers reading and respecting the comment.

• [concurrency-default-inconsistency] internal/cli/repos.go — repos diff defaults to --concurrency 8 (matching read-only commands like init and status) while repos sync defaults to 4 (matching write commands like install and upgrade). The read-vs-write split is defensible, but sync --dry-run uses 4 despite being functionally equivalent to diff.

Previous run

Review

Re-review after branch update (prior review SHA 9030ccf0, now 1bea3a97). Prior low findings have been resolved or re-evaluated: the ADR 0057 and plan document stale-status findings are resolved (both updated in this commit), the edge-case finding about ensureSecrets on the no-drift path was downgraded to info on re-evaluation (behavior is correct for convergence), and the minor-scope-addition finding (checkPerRepoScopes added to uninstall) is now informational. The implementation is well-structured: config-struct + run*() pattern, forge abstraction respected, checkPerRepoScopes gates all commands, secret values never appear in output, concurrency validation is strict (1–32), and test coverage is comprehensive (38 tests in sync_test.go, 9 integration tests in repos_test.go). Security analysis confirms correct fail-closed behavior at all auth/validation gates and proper secrets handling throughout.

Findings

Low

• [plan-implementation-deviation] internal/repos/sync.go:48 — The implementation plan (docs/plans/repos-management.md, PR 6 spec table) specifies that sync should reconcile FULLSEND_PER_REPO_INSTALL (ensure "true"), but managedVariables intentionally omits it. The code comments explain the rationale: the guard variable is managed by repos install, and diffRepo skips repos where the guard is not "true" to avoid interfering with future installs. The plan document was not updated to reflect this design decision.

• [overly-restrictive-preflight] internal/cli/repos.go — runReposDiff calls checkPerRepoScopes, which requires both repo and workflow OAuth scopes. The repos diff command is read-only (ListRepoVariables and RepoSecretExists only), so the workflow scope requirement is unnecessary. A classic PAT with repo but not workflow scope would be rejected, though installation tokens and fine-grained tokens are unaffected.

• [concurrency-default-inconsistency] internal/cli/repos.go — repos diff defaults to --concurrency 4 (matching write commands: install, upgrade, sync) despite being read-only. Read-only commands (init, status) default to 8. The pattern has a rough read-vs-write split, but diff breaks it since it shares code lineage with sync.

Previous run (2)

Review

Re-review after branch update (prior review SHA 29640148, now 9030ccf0). All prior low findings remain anchored at low severity on unchanged code patterns. The implementation is well-structured: config-struct + run*() pattern, forge abstraction respected, checkPerRepoScopes gates all commands including the newly-added uninstall preflight, secret values never appear in output, concurrency validation is strict (1–32), and test coverage is comprehensive (38+ tests in sync_test.go, 9 integration tests in repos_test.go). Security analysis confirms correct fail-closed behavior at all auth/validation gates and proper secrets handling throughout.

Findings

Low

• [edge-case] internal/repos/sync.go:265 — When diffRepo returns warnings but zero variable changes, Sync calls ensureSecrets unconditionally on the no-drift path. Correct for convergence (secrets are write-only), but the interaction between secret-check error warnings and subsequent secret writes is subtle.

• [consumer-completeness] internal/repos/sync.go:52 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install). By design per the plan — WIF provider is managed by repos install/repos upgrade, not repos sync.

• [minor-scope-addition] internal/cli/repos.go:859 — PR adds checkPerRepoScopes to runReposUninstall (an existing command not in PR scope). Defensive consistency improvement that prevents silent failures when tokens lack required permissions.

• [stale implementation status] docs/ADRs/0057-repos-management.md:137 — ADR 0057 lists repos sync and repos diff as remaining subcommands, but this PR implements both. A minor annotation to the implementation status section would keep the ADR current.

• [stale implementation status] docs/plans/repos-management.md:970 — The repos management plan's PR 6 section lacks a completion status annotation linking to this PR.

Previous run (3)

Review

Re-review after branch update (prior review SHA 7d62549d, now 29640148). Three prior medium findings are now resolved: the ! suffix has been added to the PR title, discoverRepo now logs a warning when ProbeRepoState returns an error for installed repos, and runReposUninstall now calls checkPerRepoScopes. The prior low finding about missing filterRepos glob tests is also resolved. The implementation across all commands is well-structured: config-struct + run*() pattern, forge abstraction respected, confirmGlobAction gates destructive glob operations, secret values are never logged, and test coverage is comprehensive.

Findings

Low

• [scope-creep] internal/cli/repos.go — PR title feat(repos)!: add repos diff and repos sync CLI commands mentions only diff and sync, but the PR implements five new commands (diff, sync, add, remove, uninstall). The plan document was updated to consolidate scope and the code is coherent, but the title will appear in GoReleaser release notes and may not accurately represent the full change set.

• [edge-case] internal/repos/sync.go — When diffRepo returns warnings but zero variable changes, Sync calls ensureSecrets unconditionally on the no-drift path. Correct for convergence (secrets are write-only), but the interaction between secret-check error warnings and subsequent secret writes is subtle.

• [consumer-completeness] internal/repos/sync.go — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install). By design per the plan — WIF provider is managed by repos install/repos upgrade, not repos sync.

• [naming-consistency] docs/ADRs/0057-repos-management.md — ADR 0057 lists a single repos remove command, but the implementation splits into three commands (repos add, repos remove, repos uninstall). A minor annotation to the ADR would improve discoverability.

Previous run (4)

Review

Re-review after branch update (prior review SHA 36607b76, now 7d62549d). The prior medium finding about the missing ! in the PR title remains unresolved. The implementation across all new commands is well-structured: config-struct + run*() pattern, forge abstraction respected, confirmGlobAction gates destructive glob operations, Uninstall uses correct two-phase cleanup (parallel per-repo → sequential WIF), and test coverage is comprehensive. Secret values are never logged — applyChanges filters to variable-type changes only, ensureSecrets logs only secret names, and FormatDiffTable renders secrets as "(missing)" / "(secret value)".

Findings

Medium

• [breaking-change-unmarked] PR title — The PR title feat(repos): add repos diff and repos sync CLI commands is missing the ! suffix despite a commit carrying BREAKING CHANGE: repos install no longer accepts --repo flag, requiring positional arguments instead. Per COMMITS.md, breaking changes must be marked in both commit messages and PR titles because GoReleaser builds release notes from PR titles. The commit message correctly carries feat(repos)!: with a BREAKING CHANGE: trailer, but the PR title is missing the ! suffix. This finding has been raised in prior reviews and remains unresolved.
  Remediation: Update the PR title to feat(repos)!: add repos diff and repos sync CLI commands.

• [scope-creep] internal/cli/repos.go — PR implements commands from two separate plan PRs: PR 6 (repos diff/sync) and PR 8 (repos add/remove/uninstall). The plan documents these as independent PRs developable in parallel, and the PR title only references diff/sync. The plan was updated in this PR to consolidate the scope, but the combined PR is substantial (~5000 lines, 20 files) and the title does not reflect the full scope of changes.

• [error-handling] internal/repos/init.go:328 — Refactoring discoverRepo to use ProbeRepoState silently drops workflow-read errors for installed repos. When ProbeRepoState returns Installed=true with a non-nil error (workflow file unreadable), the condition err != nil && !state.Installed is false, so execution continues with FullsendRef="". During repos init, this generates a manifest entry without a fullsend_ref override, silently degrading manifest quality compared to the prior behavior which returned a hard error.
  Remediation: Log a progress warning when ProbeRepoState returns an error for an installed repo, e.g., progress(fullName, "discover", fmt.Sprintf("warning: %v", err)).

Low

• [naming-consistency] internal/cli/repos.go — ADR 0057 lists a single repos remove command for teardown, but the implementation splits into repos remove (manifest-only) and repos uninstall (teardown). The split is a reasonable design evolution, but the ADR should be updated to reflect the three-command design.

• [authorization] internal/cli/repos.go:873 — runReposUninstall performs destructive operations (delete workflow, variables, secrets, WIF) but does not call checkPerRepoScopes before proceeding, unlike repos install, repos diff, and repos sync. Users with insufficient permissions discover the problem mid-operation, potentially leaving repos in a partially uninstalled state.

• [edge-case] internal/repos/sync.go:236 — When diffRepo returns warnings but zero variable changes, Sync calls ensureSecrets unconditionally on the no-drift path. Correct for convergence (secrets are write-only) but the warning path is subtle.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install). By design per the plan — WIF provider is managed by repos upgrade, not repos sync.

• [test-adequacy] internal/repos/status.go:204 — filterRepos now uses matchesPattern for glob support but direct unit tests only cover exact matching. Glob behavior tested indirectly via MatchManifestRepos in manifest_edit_test.go.

• [documentation-drift] docs/cli/repos.md — Three commands (add/remove/uninstall) where ADR 0057 authorized two. The docs are internally consistent but the ADR is stale.

Previous run (5)

Review

This is a re-review after the branch was rebased (prior review SHA 3d80470, now 36607b76). The prior review's medium finding about the missing ! in the PR title remains unresolved. All low findings from the prior review are carried forward with anchored severities on unchanged code.

The implementation across all new commands is well-structured: repos add, repos remove, and repos uninstall follow the config-struct + run*() pattern consistently, the forge abstraction is fully respected, confirmGlobAction correctly gates destructive glob operations with terminal detection, Uninstall uses the correct two-phase pattern (parallel per-repo cleanup → sequential WIF), and the WIF provider leak fix in splitProjectAdapter.DeletePerRepoWIF uses correct ordering (mint deregistration first, then WIF provider deletion). The ProbeRepoState refactoring in status.go correctly consolidates variable reading and workflow ref extraction. Secret values are never logged — applyChanges filters to variable-type changes only, ensureSecrets logs only secret names, and FormatDiffTable renders secrets as "(missing)" / "(secret value)". All 10+ WIFProvisioner implementations are updated with DeleteWIFProvider. Test coverage is comprehensive across all new files.

Findings

Medium

• [breaking-change-unmarked] PR title — The PR title is feat(repos): add repos diff and repos sync CLI commands but commit 8867437f contains a BREAKING CHANGE: repos install no longer accepts --repo flag, requiring positional arguments instead. Per COMMITS.md: "Breaking changes must be marked in both commit messages and PR titles. Flag a missing ! as an important-severity finding. The PR title is especially critical because GoReleaser uses it to build user-facing release notes." The commit message correctly carries feat(repos)!: with a BREAKING CHANGE: trailer, but the PR title is missing the ! suffix. This finding was raised in the prior review and remains unresolved.
  Remediation: Update the PR title to include !, e.g., feat(repos)!: add repos diff, sync, add, remove, and uninstall subcommands.

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., RepoSecretExists failure) with no variable changes, Sync() skips ensureSecrets on the no-drift path due to the if len(diffWarnings) == 0 guard, and does not mark rr.failed = true. The repo's secrets are silently left unreconciled. This is consistent with how Diff() handles the same scenario (warnings, not errors), so the behavior is defensible — ensure-style convergence is skipped when the API state is uncertain.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

• [test-adequacy] internal/repos/status.go — filterRepos was changed from exact map-based lookup to iterating with matchesPattern (adding glob support), but existing tests for filterRepos in status_test.go only cover exact matching and case-insensitive matching. Glob matching is tested indirectly via MatchManifestRepos in manifest_edit_test.go, but there is no direct filterRepos test with glob patterns.

---

Labels: PR adds new CLI subcommands in the repos package and modifies the WIFProvisioner interface

Previous run (6)

Review

This is a re-review after new commits were pushed. The prior review (SHA 94e0b1a) covered only the repos diff and repos sync code and approved with low findings. Since then, a new commit (47bb282) was added that introduces repos add, repos remove, repos uninstall, changes repos install to use positional args (breaking change), adds DeleteWIFProvider to the WIFProvisioner interface, fixes a pre-existing WIF provider leak in splitProjectAdapter.DeletePerRepoWIF, refactors discoverRepo to use the new ProbeRepoState helper, and adds glob support to filterRepos. This review covers all new code.

The implementation is well-structured across all new commands: the config-struct + run*() pattern is followed consistently, the forge abstraction is fully respected, confirmGlobAction correctly gates destructive glob operations with terminal detection, Uninstall uses the correct two-phase pattern (parallel per-repo cleanup → sequential WIF), concurrency validation is properly enforced (1–32 bounds), and test coverage is comprehensive across all new files. The ProbeRepoState refactoring in status.go correctly consolidates variable reading and workflow ref extraction, and resolveConfigWithGlobs in uninstall.go correctly handles glob-matched manifest entries for WIF cleanup.

Findings

Medium

• [breaking-change-unmarked] PR title — The PR title is feat(repos): add repos diff and repos sync CLI commands but commit 47bb282 contains a BREAKING CHANGE: repos install no longer accepts --repo flag, requiring positional arguments instead. Per COMMITS.md: "Breaking changes must be marked in both commit messages and PR titles. Flag a missing ! as an important-severity finding. The PR title is especially critical because GoReleaser uses it to build user-facing release notes." The commit message correctly carries feat(repos)!: with a BREAKING CHANGE: trailer, but the PR title is missing the ! suffix.
  Remediation: Update the PR title to include !, e.g., feat(repos)!: add repos diff, sync, add, remove, and uninstall subcommands.

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., RepoSecretExists failure) with no variable changes, Sync() skips ensureSecrets on the no-drift path due to the if len(diffWarnings) == 0 guard, and does not mark rr.failed = true. The repo's secrets are silently left unreconciled. This is consistent with how Diff() handles the same scenario (warnings, not errors), so the behavior is defensible — ensure-style convergence is skipped when the API state is uncertain.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

• [test-adequacy] internal/repos/status.go — filterRepos was changed from exact map-based lookup to iterating with matchesPattern (adding glob support), but existing tests for filterRepos in status_test.go only cover exact matching and case-insensitive matching. Glob matching is tested indirectly via MatchManifestRepos in manifest_edit_test.go, but there is no direct filterRepos test with glob patterns.

Previous run (7)

Review

This is a re-review after new commits were pushed. The prior review (SHA 94e0b1a) covered only the repos diff and repos sync code and approved with low findings. Since then, a new commit (47bb282) was added that introduces repos add, repos remove, repos uninstall, changes repos install to use positional args (breaking change), adds DeleteWIFProvider to the WIFProvisioner interface, fixes a pre-existing WIF provider leak in splitProjectAdapter.DeletePerRepoWIF, refactors discoverRepo to use the new ProbeRepoState helper, and adds glob support to filterRepos. This review covers all new code.

The implementation is well-structured across all new commands: the config-struct + run*() pattern is followed consistently, the forge abstraction is fully respected, confirmGlobAction correctly gates destructive glob operations with terminal detection, Uninstall uses the correct two-phase pattern (parallel per-repo cleanup → sequential WIF), concurrency validation is properly enforced (1–32 bounds), and test coverage is comprehensive across all new files. The ProbeRepoState refactoring in status.go correctly consolidates variable reading and workflow ref extraction, and resolveConfigWithGlobs in uninstall.go correctly handles glob-matched manifest entries for WIF cleanup.

Findings

Medium

• [breaking-change-unmarked] PR title — The PR title is feat(repos): add repos diff and repos sync CLI commands but commit 47bb282 contains a BREAKING CHANGE: repos install no longer accepts --repo flag, requiring positional arguments instead. Per COMMITS.md: "Breaking changes must be marked in both commit messages and PR titles. Flag a missing ! as an important-severity finding. The PR title is especially critical because GoReleaser uses it to build user-facing release notes." The commit message correctly carries feat(repos)!: with a BREAKING CHANGE: trailer, but the PR title is missing the ! suffix.
  Remediation: Update the PR title to include !, e.g., feat(repos)!: add repos diff, sync, add, remove, and uninstall subcommands.

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., RepoSecretExists failure) with no variable changes, Sync() skips ensureSecrets on the no-drift path due to the if len(diffWarnings) == 0 guard, and does not mark rr.failed = true. The repo's secrets are silently left unreconciled. This is consistent with how Diff() handles the same scenario (warnings, not errors), so the behavior is defensible — ensure-style convergence is skipped when the API state is uncertain.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

• [test-adequacy] internal/repos/status.go — filterRepos was changed from exact map-based lookup to iterating with matchesPattern (adding glob support), but existing tests for filterRepos in status_test.go only cover exact matching and case-insensitive matching. Glob matching is tested indirectly via MatchManifestRepos in manifest_edit_test.go, but there is no direct filterRepos test with glob patterns.

Previous run (8)

Review

Three prior low findings from the last review have been addressed: concurrency flag descriptions now consistently say "max parallel operations (1-32)" (matching repos install), the text-mode summary now includes failure count ("Applied N changes, M repos failed."), and a doc-comment on managedVariables warns that entries are logged in cleartext. No changes to core logic or tests.

The implementation remains well-structured: the config-struct + run*() pattern matches existing CLI commands, Diff() and Sync() both use ResolveConfigForEntry for correct glob-expanded config resolution, the semaphore-bounded concurrency model matches repos status / repos install, the forge abstraction is fully respected, secret values never appear in output, and test coverage is comprehensive (37+ tests, 93–100% coverage). PR title follows COMMITS.md conventions correctly (feat(repos) for new user-facing CLI commands).

Findings

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., RepoSecretExists failure) with no variable changes, Sync() skips ensureSecrets on the no-drift path due to the if len(diffWarnings) == 0 guard, and does not mark rr.failed = true. The repo's secrets are silently left unreconciled. This is consistent with how Diff() handles the same scenario (warnings, not errors), so the behavior is defensible — ensure-style convergence is skipped when the API state is uncertain.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

Previous run (9)

Review

Three prior low findings from the last review have been addressed: concurrency flag descriptions now consistently say "max parallel operations (1-32)" (matching repos install), the text-mode summary now includes failure count ("Applied N changes, M repos failed."), and a doc-comment on managedVariables warns that entries are logged in cleartext. No changes to core logic or tests.

The implementation remains well-structured: the config-struct + run*() pattern matches existing CLI commands, Diff() and Sync() both use ResolveConfigForEntry for correct glob-expanded config resolution, the semaphore-bounded concurrency model matches repos status / repos install, the forge abstraction is fully respected, secret values never appear in output, and test coverage is comprehensive (37+ tests, 93–100% coverage). PR title follows COMMITS.md conventions correctly (feat(repos) for new user-facing CLI commands).

Findings

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., RepoSecretExists failure) with no variable changes, Sync() skips ensureSecrets on the no-drift path due to the if len(diffWarnings) == 0 guard, and does not mark rr.failed = true. The repo’s secrets are silently left unreconciled. This is consistent with how Diff() handles the same scenario (warnings, not errors), so the behavior is defensible — ensure-style convergence is skipped when the API state is uncertain.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

Previous run (10)

Review

All prior medium+ findings from earlier review iterations are resolved: Sync() correctly uses ResolveConfigForEntry (not ResolveConfig) for glob-expanded repos, applyChanges() filters to variable-type changes only and delegates all secret writes to ensureSecrets(), concurrency validation uses validateConcurrency() with strict 1–32 bounds, and Diff() only reports missing secrets (not existing ones). Documentation has been added to all three doc files. The repos sync CLI now calls checkPerRepoScopes() for token scope verification.

The implementation is well-structured: the config-struct + run*() pattern matches existing CLI commands, the semaphore-bounded concurrency model matches repos status / repos install, the forge abstraction is fully respected (all operations go through forge.Client), secret values never appear in output (progress messages log only secret names, FormatDiffTable renders secrets as "(missing)" / "(secret value)"), and test coverage is comprehensive (37+ tests, 93–100% coverage).

Findings

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., RepoSecretExists failure) with no variable changes, Sync() skips ensureSecrets on the no-drift path due to the if len(diffWarnings) == 0 guard, and does not mark rr.failed = true. The repo's secrets are silently left unreconciled. This is consistent with how Diff() handles the same scenario (warnings, not errors), so the behavior is defensible — ensure-style convergence is skipped when the API state is uncertain.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

• [error-handling] internal/cli/repos.go:651 — In runReposSync text mode, when Sync returns a partial failure (err != nil, result != nil), the Applied N changes line and warnings are printed, but the output could mislead CI pipelines. The partial-failure summary goes to stdout while the error goes to stderr, with no differentiation.
  Remediation: Consider including the failure count in the summary line (e.g., Applied N changes, M repos failed.) or printing warnings to stderr.

• [information-exposure] internal/repos/sync.go:325 — The applyChanges progress callback logs variable values in cleartext via fmt.Sprintf("%s %s=%s", c.Action, c.Field, c.NewValue). Currently safe because only non-sensitive variables (FULLSEND_MINT_URL, FULLSEND_GCP_REGION) pass through this path, but lacks defense-in-depth if a future developer adds a sensitive variable to managedVariables instead of managedSecrets.
  Remediation: Either remove the value from the progress message or add a doc-comment on managedVariables warning that entries must not contain sensitive data.

• [flag-consistency] internal/cli/repos.go:149,221 — Concurrency flag description for repos diff/sync uses "max parallel API calls" while repos install uses "max parallel operations (1-32)". Minor inconsistency.

Previous run (11)

Review

Findings

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., ListRepoVariables failure) with no variable changes, Sync() skips all reconciliation for that repo but does not mark rr2.failed = true. Sync returns exit code 0 and the warning is visible in output, but CI pipelines relying solely on exit code would not detect the incomplete sync. This is consistent with how Diff() handles the same scenario, so the behavior is defensible.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

• [error-handling] internal/cli/repos.go:655 — In runReposSync text mode, when Sync returns a partial failure (err != nil, result != nil), the Applied N changes summary is suppressed because the else if err == nil branch is skipped. Per-repo progress messages are visible via the callback, but the final summary line is lost.
  Remediation: Consider printing warnings from result.Warnings in text mode even when err != nil, similar to how JSON mode always prints the full result.

• [authorization] internal/cli/repos.go:595 — repos sync and repos diff skip the checkPerRepoScopes() preflight that repos install performs. Users with insufficient token scopes get opaque API errors instead of a clear preflight failure. Not a privilege escalation (GitHub enforces scopes server-side) but a defense-in-depth gap.
  Remediation: Call checkPerRepoScopes(ctx, client, printer) at the top of runReposSync, matching the pattern established by runReposInstall.

• [naming-convention] internal/repos/sync.go:125 — Variable name rr2 in the Sync goroutine is opaque alongside the rr (ResolvedRepo) closure parameter. Consider renaming to repoRes or similar for clarity.

Previous run (12)

Review

All prior medium findings are resolved: applyChanges() now correctly filters to variable-type changes only and delegates all secret writes to ensureSecrets() (eliminating duplicate writes), nil-result handling in the CLI guards result == nil before accessing it, and the API-error fallthrough to secret writes is fixed by the len(diffWarnings) == 0 guard. Documentation for all three doc files has been added.

The implementation is well-structured: Diff() and Sync() correctly use ResolveConfigForEntry (not ResolveConfig) for glob-expanded repos, concurrency is validated with strict 1–32 bounds via validateConcurrency(), the semaphore-bounded concurrency pattern matches existing repos status/repos install, and test coverage is thorough (37+ tests covering edge cases, glob expansion with per-entry overrides, API errors, and concurrency validation).

Findings

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., ListRepoVariables failure) with no variable changes, Sync() skips all reconciliation for that repo but does not mark rr2.failed = true. Sync returns exit code 0 and the warning is visible in output, but CI pipelines relying solely on exit code would not detect the incomplete sync. This is consistent with how Diff() handles the same scenario (warnings, not errors), so the behavior is defensible — but it means Sync can report success for repos it never actually converged.
  Remediation: Consider marking rr2.failed = true when diffWarnings are present and changes are empty, or document that exit code 0 only guarantees that attempted reconciliations succeeded.

• [test-adequacy] internal/repos/sync_test.go — No test covers the Sync path where diffRepo returns API warnings (e.g., ListRepoVariables error with no changes). The len(changes)==0 && len(diffWarnings)>0 branch is untested.
  Remediation: Add a test like TestSync_DiffAPIError_SkipsReconciliation that sets fc.Errors["ListRepoVariables"] and verifies the result includes warnings and the expected Failed count.

• [authorization] internal/cli/repos.go — repos sync writes secrets and variables but does not call checkPerRepoScopes() like repos install does (line 370). Users with insufficient token scopes get opaque API errors instead of a clear preflight failure. Not a privilege escalation (GitHub enforces scopes server-side) but a defense-in-depth gap.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

Previous run (13)

Review

All prior high and medium findings have been addressed: glob-expanded repo config resolution now uses ResolveConfigForEntry in both Diff and Sync, concurrency validation uses validateConcurrency() with strict 1–32 bounds, and Diff now only reports missing secrets rather than always reporting existing ones as drift.

Findings

Medium

• [logic-error] internal/repos/sync.go:336 — applyChanges() writes a missing secret via resolveSecretValue (line 328), then unconditionally calls ensureSecrets() (line 336) which writes the same secret again. This produces duplicate entries in the Applied list and duplicate CreateRepoSecret API calls. The writes are idempotent (no data corruption), but duplicate Applied entries misrepresent the number of changes.
  Remediation: Either skip secrets in the applyChanges loop (let ensureSecrets handle all secret writes) or pass applied secret names to ensureSecrets to skip already-written secrets.

Low

• [error-handling] internal/cli/repos.go:647 — When Sync() fails early (validation, glob expansion), result is nil. The JSON output path runs unconditionally when --json is set, outputting "null" before returning the error.

• [edge-case] internal/repos/sync.go:244 — When diffRepo returns warnings but no changes (API error), the code falls through to applyChanges → ensureSecrets, issuing secret-write API calls against a repo that just failed basic API access.

• [authorization] internal/cli/repos.go — repos install calls checkPerRepoScopes() before API operations; repos sync (which writes the same secrets and variables) does not, leading to cryptic 403s instead of a clear permissions error.

• [missing-documentation] docs/cli/repos.md:10, docs/guides/getting-started/operations.md:74, docs/guides/dev/cli-internals.md:41 — All three doc files list only init, install, status under repos subcommands. The new diff and sync commands are not documented.

• [pattern-inconsistency] internal/cli/repos.go — repos diff/sync default to --concurrency 4 while repos status/init default to 8. The pattern matches repos install (also 4) and appears intentional for write-path commands, but is undocumented.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install). By design per the plan, but WIF provider drift will not be detected by repos sync.

Previous run (14)

Review

Well-structured feature PR that adds repos diff and repos sync CLI commands following established patterns in the repos package. The concurrency model, forge abstraction compliance, and test coverage are solid. However, one high-severity bug from the prior review remains unfixed and must be addressed before merge.

High: Wrong config resolution for glob-expanded repos in Sync

Sync() (sync.go, line 246) uses manifest.ResolveConfig(k.owner, k.repo) to resolve configuration when applying changes. ResolveConfig (manifest.go:503-514) performs an exact-match lookup against the manifest's repo list — its doc comment states: "this method only finds exact matches in the manifest's repo list and will not match glob patterns."

For glob-expanded repos (e.g., acme/* expanding to acme/api), ResolveConfig iterates m.Repos looking for e.Repo == "acme/api", but the manifest entry is "acme/*". The match fails and the method falls back to resolveWithEntry(owner, repo, RepoEntry{}), which builds config from manifest defaults only — discarding per-glob overrides (inference_project, inference_region). The discarded second return value (cfg, _ := manifest.ResolveConfig(...)) hides the miss.

Diff() correctly uses manifest.ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry) (sync.go, line 96), which preserves per-entry overrides. This creates a split-brain: Diff reports the correct desired state, but Sync applies different (incorrect) values via applyChanges → resolveSecretValue. Since secrets are always re-synced (they can't be read back for comparison), every Sync run on glob-expanded repos with per-entry inference_project overrides will write the wrong GCP project ID as a repo secret.

Remediation: Propagate the ResolvedRepo.Entry (or the already-resolved ResolvedConfig) from the Diff phase through to applyChanges. Options:

1. Add an Entry RepoEntry or Config ResolvedConfig field to the Change struct so applyChanges has the correct per-entry config.
2. Restructure Sync to call ExpandGlobs + ResolveConfigForEntry itself rather than re-resolving through ResolveConfig.
3. At minimum, replace manifest.ResolveConfig(k.owner, k.repo) with a lookup that consults the glob-expanded entries.

Medium: Concurrency bounds not validated

Diff and Sync use if maxConcurrency < 1 { maxConcurrency = 4 } (silent correction), while BatchInstall validates bounds strictly (if cfg.MaxConcurrency <= 0 || cfg.MaxConcurrency > 32 { return error }), and the repos install CLI validates 1-32. The new code accepts invalid values (negative or unbounded) silently. Note: Status uses the same silent-correction pattern, so this is a codebase-wide inconsistency rather than a new regression.

Low findings

• Secrets always reported as drift: Existing secrets always produce an "update" Change since values can't be read for comparison. Every diff run shows secret drift even when nothing changed, and every sync run re-writes secrets. This is a known limitation of the GitHub Secrets API — the behavior ensures convergence but produces noisy output.
• sync --dry-run exit code inconsistency: repos diff returns exit code 1 when drift exists; repos sync --dry-run returns 0 after showing the same output. CI scripts cannot use sync --dry-run as a drop-in replacement for diff as a drift check.
• Guard variable in managedVariables is dead code: diffRepo exits early when guard ≠ "true", so by the time managedVariables is iterated, the guard is guaranteed "true" — matching the hardcoded desired value. No Change is ever emitted for this entry.
• Documentation not updated: docs/cli/repos.md, docs/guides/getting-started/operations.md, and docs/guides/dev/cli-internals.md list only init, install, status — missing the new diff and sync commands. The CLI --help text is present, so this is not blocking, but the docs should be updated as a follow-up.
• Concurrency default inconsistency: repos diff and repos sync default to --concurrency 4; repos status defaults to 8. May be intentional (write vs read operations) but is undocumented.

---

Labels: PR adds new CLI subcommands (repos diff, repos sync) in the repos package

Previous run (15)

Review

Well-structured feature PR that adds repos diff and repos sync CLI commands following the established patterns in the repos package. The concurrency model, forge abstraction usage, and test coverage are all solid. However, one high-severity bug needs to be fixed before merge.

High: Wrong config resolution for glob-expanded repos in Sync

Sync() (sync.go) uses manifest.ResolveConfig(k.owner, k.repo) to resolve configuration when applying changes. ResolveConfig only matches repos by exact name — its own doc comment states: "this method only finds exact matches in the manifest's repo list and will not match glob patterns."

For glob-expanded repos (e.g., acme-corp/*), ResolveConfig won't find the entry and falls back to a ResolvedConfig built from manifest defaults only — dropping per-glob overrides (e.g., inference_project, inference_region). This means resolveSecretValue() could write the wrong GCP project ID as a repo secret.

Diff() correctly uses manifest.ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry), which preserves per-glob overrides. This creates an inconsistency: Diff shows the correct desired state, but Sync applies different (incorrect) values.

Remediation: Either propagate the ResolvedRepo.Entry through the sync path (e.g., attach it to the Change struct or group by ResolvedRepo instead of repoKey), or restructure Sync to iterate over ResolvedRepo entries from ExpandGlobs directly instead of re-resolving config after Diff.

Medium: Partial changes applied on secret check error

In diffRepo, when client.RepoSecretExists fails (sync.go), the function returns the partial changes slice accumulated so far alongside a warning. Sync then applies those partial variable changes while the failed secret is silently skipped and only appears as a warning. This could leave a repo in an inconsistent state where variables are updated but secrets are not.

Remediation: Either return nil changes on secret check failure (preventing partial application), or continue iterating through remaining secrets and collect all warnings.

Medium: Diff always reports secret updates (noisy output)

diffRepo always emits an "update" Change for existing secrets since their values cannot be read for comparison. This means repos diff can never show "No changes needed" for any installed repo with managed secrets, even when the secret values are correct. The TestDiff_NoDrift test acknowledges this by only asserting on variable changes.

Remediation: Consider separating the "ensure secret exists" concept from "update secret value." For diff, only report create for missing secrets. For sync, offer a --force-secrets flag to re-write existing secrets.

Medium: CLI docs not updated

docs/cli/repos.md lists only three repos subcommands (init, install, status). The new diff and sync commands are missing from the commands table and have no documentation sections. Similarly, docs/guides/getting-started/operations.md lists repos commands in the standalone commands table but omits diff and sync.

Low: Test coverage gap for glob + per-entry overrides in Sync

TestSync_GlobExpansion uses a glob pattern but without per-entry overrides, so the ResolveConfig bug isn't caught (manifest defaults produce the same result). A test with per-glob inference_project or inference_region overrides would expose the high-severity bug above.

Low: Concurrency default inconsistency

repos diff and repos sync default to --concurrency 4, while repos status defaults to 8 and repos init defaults to 8. The inconsistency may be intentional (write operations vs read-only), but it's undocumented.

Low: Guard variable in managedVariables is dead code

The managedVariables slice includes forge.PerRepoGuardVar with a resolve function returning "true". Since diffRepo skips repos where guard != "true", the guard can never differ from the desired value — it's always "true" when evaluated. This entry will never produce a Change.

Low: No upper bound on --concurrency

The diff and sync commands accept arbitrary --concurrency values without an upper bound, unlike repos install which validates 1–32. While the practical impact is minimal (goroutine count is bounded by repo count), consistency with install's validation would be good.

---

Labels: PR adds new CLI commands in the repos package

Previous run (16)

Review

All prior high and medium findings have been addressed: glob-expanded repo config resolution now uses ResolveConfigForEntry in both Diff and Sync, concurrency validation uses validateConcurrency() with strict 1–32 bounds, and Diff now only reports missing secrets rather than always reporting existing ones as drift.

Findings

Medium

• [logic-error] internal/repos/sync.go:336 — applyChanges() writes a missing secret via resolveSecretValue (line 328), then unconditionally calls ensureSecrets() (line 336) which writes the same secret again. This produces duplicate entries in the Applied list and duplicate CreateRepoSecret API calls. The writes are idempotent (no data corruption), but duplicate Applied entries misrepresent the number of changes.
  Remediation: Either skip secrets in the applyChanges loop (let ensureSecrets handle all secret writes) or pass applied secret names to ensureSecrets to skip already-written secrets.

Low

• [error-handling] internal/cli/repos.go:647 — When Sync() fails early (validation, glob expansion), result is nil. The JSON output path runs unconditionally when --json is set, outputting "null" before returning the error.

• [edge-case] internal/repos/sync.go:244 — When diffRepo returns warnings but no changes (API error), the code falls through to applyChanges → ensureSecrets, issuing secret-write API calls against a repo that just failed basic API access.

• [authorization] internal/cli/repos.go — repos install calls checkPerRepoScopes() before API operations; repos sync (which writes the same secrets and variables) does not, leading to cryptic 403s instead of a clear permissions error.

• [missing-documentation] docs/cli/repos.md:10, docs/guides/getting-started/operations.md:74, docs/guides/dev/cli-internals.md:41 — All three doc files list only init, install, status under repos subcommands. The new diff and sync commands are not documented.

• [pattern-inconsistency] internal/cli/repos.go — repos diff/sync default to --concurrency 4 while repos status/init default to 8. The pattern matches repos install (also 4) and appears intentional for write-path commands, but is undocumented.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install). By design per the plan, but WIF provider drift will not be detected by repos sync.

Previous run (17)

Review

Well-structured feature PR that adds repos diff and repos sync CLI commands following established patterns in the repos package. The concurrency model, forge abstraction compliance, and test coverage are solid. However, one high-severity bug from the prior review remains unfixed and must be addressed before merge.

High: Wrong config resolution for glob-expanded repos in Sync

Sync() (sync.go, line 246) uses manifest.ResolveConfig(k.owner, k.repo) to resolve configuration when applying changes. ResolveConfig (manifest.go:503-514) performs an exact-match lookup against the manifest's repo list — its doc comment states: "this method only finds exact matches in the manifest's repo list and will not match glob patterns."

For glob-expanded repos (e.g., acme/* expanding to acme/api), ResolveConfig iterates m.Repos looking for e.Repo == "acme/api", but the manifest entry is "acme/*". The match fails and the method falls back to resolveWithEntry(owner, repo, RepoEntry{}), which builds config from manifest defaults only — discarding per-glob overrides (inference_project, inference_region). The discarded second return value (cfg, _ := manifest.ResolveConfig(...)) hides the miss.

Diff() correctly uses manifest.ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry) (sync.go, line 96), which preserves per-entry overrides. This creates a split-brain: Diff reports the correct desired state, but Sync applies different (incorrect) values via applyChanges → resolveSecretValue. Since secrets are always re-synced (they can't be read back for comparison), every Sync run on glob-expanded repos with per-entry inference_project overrides will write the wrong GCP project ID as a repo secret.

Remediation: Propagate the ResolvedRepo.Entry (or the already-resolved ResolvedConfig) from the Diff phase through to applyChanges. Options:

1. Add an Entry RepoEntry or Config ResolvedConfig field to the Change struct so applyChanges has the correct per-entry config.
2. Restructure Sync to call ExpandGlobs + ResolveConfigForEntry itself rather than re-resolving through ResolveConfig.
3. At minimum, replace manifest.ResolveConfig(k.owner, k.repo) with a lookup that consults the glob-expanded entries.

Medium: Concurrency bounds not validated

Diff and Sync use if maxConcurrency < 1 { maxConcurrency = 4 } (silent correction), while BatchInstall validates bounds strictly (if cfg.MaxConcurrency <= 0 || cfg.MaxConcurrency > 32 { return error }), and the repos install CLI validates 1-32. The new code accepts invalid values (negative or unbounded) silently. Note: Status uses the same silent-correction pattern, so this is a codebase-wide inconsistency rather than a new regression.

Low findings

• Secrets always reported as drift: Existing secrets always produce an "update" Change since values can't be read for comparison. Every diff run shows secret drift even when nothing changed, and every sync run re-writes secrets. This is a known limitation of the GitHub Secrets API — the behavior ensures convergence but produces noisy output.
• sync --dry-run exit code inconsistency: repos diff returns exit code 1 when drift exists; repos sync --dry-run returns 0 after showing the same output. CI scripts cannot use sync --dry-run as a drop-in replacement for diff as a drift check.
• Guard variable in managedVariables is dead code: diffRepo exits early when guard ≠ "true", so by the time managedVariables is iterated, the guard is guaranteed "true" — matching the hardcoded desired value. No Change is ever emitted for this entry.
• Documentation not updated: docs/cli/repos.md, docs/guides/getting-started/operations.md, and docs/guides/dev/cli-internals.md list only init, install, status — missing the new diff and sync commands. The CLI --help text is present, so this is not blocking, but the docs should be updated as a follow-up.
• Concurrency default inconsistency: repos diff and repos sync default to --concurrency 4; repos status defaults to 8. May be intentional (write vs read operations) but is undocumented.

---

Labels: PR adds new CLI subcommands (repos diff, repos sync) in the repos package

Previous run (18)

Review

Well-structured feature PR that adds repos diff and repos sync CLI commands following the established patterns in the repos package. The concurrency model, forge abstraction usage, and test coverage are all solid. However, one high-severity bug needs to be fixed before merge.

High: Wrong config resolution for glob-expanded repos in Sync

Sync() (sync.go) uses manifest.ResolveConfig(k.owner, k.repo) to resolve configuration when applying changes. ResolveConfig only matches repos by exact name — its own doc comment states: "this method only finds exact matches in the manifest's repo list and will not match glob patterns."

For glob-expanded repos (e.g., acme-corp/*), ResolveConfig won't find the entry and falls back to a ResolvedConfig built from manifest defaults only — dropping per-glob overrides (e.g., inference_project, inference_region). This means resolveSecretValue() could write the wrong GCP project ID as a repo secret.

Diff() correctly uses manifest.ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry), which preserves per-glob overrides. This creates an inconsistency: Diff shows the correct desired state, but Sync applies different (incorrect) values.

Remediation: Either propagate the ResolvedRepo.Entry through the sync path (e.g., attach it to the Change struct or group by ResolvedRepo instead of repoKey), or restructure Sync to iterate over ResolvedRepo entries from ExpandGlobs directly instead of re-resolving config after Diff.

Medium: Partial changes applied on secret check error

In diffRepo, when client.RepoSecretExists fails (sync.go), the function returns the partial changes slice accumulated so far alongside a warning. Sync then applies those partial variable changes while the failed secret is silently skipped and only appears as a warning. This could leave a repo in an inconsistent state where variables are updated but secrets are not.

Remediation: Either return nil changes on secret check failure (preventing partial application), or continue iterating through remaining secrets and collect all warnings.

Medium: Diff always reports secret updates (noisy output)

diffRepo always emits an "update" Change for existing secrets since their values cannot be read for comparison. This means repos diff can never show "No changes needed" for any installed repo with managed secrets, even when the secret values are correct. The TestDiff_NoDrift test acknowledges this by only asserting on variable changes.

Remediation: Consider separating the "ensure secret exists" concept from "update secret value." For diff, only report create for missing secrets. For sync, offer a --force-secrets flag to re-write existing secrets.

Medium: CLI docs not updated

docs/cli/repos.md lists only three repos subcommands (init, install, status). The new diff and sync commands are missing from the commands table and have no documentation sections. Similarly, docs/guides/getting-started/operations.md lists repos commands in the standalone commands table but omits diff and sync.

Low: Test coverage gap for glob + per-entry overrides in Sync

TestSync_GlobExpansion uses a glob pattern but without per-entry overrides, so the ResolveConfig bug isn't caught (manifest defaults produce the same result). A test with per-glob inference_project or inference_region overrides would expose the high-severity bug above.

Low: Concurrency default inconsistency

repos diff and repos sync default to --concurrency 4, while repos status defaults to 8 and repos init defaults to 8. The inconsistency may be intentional (write operations vs read-only), but it's undocumented.

Low: Guard variable in managedVariables is dead code

The managedVariables slice includes forge.PerRepoGuardVar with a resolve function returning "true". Since diffRepo skips repos where guard != "true", the guard can never differ from the desired value — it's always "true" when evaluated. This entry will never produce a Change.

Low: No upper bound on --concurrency

The diff and sync commands accept arbitrary --concurrency values without an upper bound, unlike repos install which validates 1–32. While the practical impact is minimal (goroutine count is bounded by repo count), consistency with install's validation would be good.

---

Labels: PR adds new CLI commands in the repos package

Previous run (19)

Review

Findings

Low

• [edge-case] internal/repos/sync.go:239 — When diffRepo returns an API warning (e.g., ListRepoVariables failure) with no variable changes, Sync() skips all reconciliation for that repo but does not mark rr2.failed = true. Sync returns exit code 0 and the warning is visible in output, but CI pipelines relying solely on exit code would not detect the incomplete sync. This is consistent with how Diff() handles the same scenario, so the behavior is defensible.

• [consumer-completeness] internal/repos/sync.go:50 — managedSecrets does not include FULLSEND_GCP_WIF_PROVIDER (written by repos install at install.go:275). By design per the plan — WIF provider is provisioned at install time and managed by repos upgrade — but WIF provider drift will not be detected by repos sync.

• [error-handling] internal/cli/repos.go:655 — In runReposSync text mode, when Sync returns a partial failure (err != nil, result != nil), the Applied N changes summary is suppressed because the else if err == nil branch is skipped. Per-repo progress messages are visible via the callback, but the final summary line is lost.
  Remediation: Consider printing warnings from result.Warnings in text mode even when err != nil, similar to how JSON mode always prints the full result.

• [authorization] internal/cli/repos.go:595 — repos sync and repos diff skip the checkPerRepoScopes() preflight that repos install performs. Users with insufficient token scopes get opaque API errors instead of a clear preflight failure. Not a privilege escalation (GitHub enforces scopes server-side) but a defense-in-depth gap.
  Remediation: Call checkPerRepoScopes(ctx, client, printer) at the top of runReposSync, matching the pattern established by runReposInstall.

• [naming-convention] internal/repos/sync.go:125 — Variable name rr2 in the Sync goroutine is opaque alongside the rr (ResolvedRepo) closure parameter. Consider renaming to repoRes or similar for clarity.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:55 AM UTC · Completed 1:06 AM UTC
Commit: e8e581b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:54 AM UTC · Completed 2:10 AM UTC
Commit: f71d575 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:17 AM UTC · Completed 2:30 AM UTC
Commit: 466a3e0 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:34 AM UTC · Completed 2:47 AM UTC
Commit: 77aa6e4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:05 PM UTC · Completed 8:19 PM UTC
Commit: 1bea3a9 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:28 PM UTC · Completed 8:44 PM UTC
Commit: 3c64248 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 9:08 PM UTC · Completed 9:20 PM UTC
Commit: 3c64248 · View workflow run →

[fullsend-ai-retro]

Retrospective analysis of PR #4079 — a human-authored PR adding repos diff and repos sync CLI commands (1,880 lines across 4 files). The review agent ran 16 times over 7 days, catching legitimate code-level bugs in early rounds (glob config resolution, duplicate secret writes, misleading diff output). However, the human reviewer (waynesun09) caught all high-impact findings: a CRITICAL spec-compliance violation (guard-variable reconciliation missing per the plan spec), a HIGH regression introduced by the fix itself (bricking future repos install), and several MEDIUM findings (missing JSON test coverage, schema inconsistency between --dry-run and normal mode, stale PR description claims). The review agent missed all of these across 9 pre-fix runs and approved the broken fix immediately. Root cause: the agent never read the plan document (docs/plans/repos-management.md) despite the PR description explicitly linking to it with "Implements PR 6 from the repos management plan." Additionally, the agent's re-review system verified that the original finding was resolved but did not analyze the fix's side effects on other code paths. Stale low-severity findings (WIF provider consumer-completeness, edge-case on ensureSecrets) were repeated identically across all 16 runs — a known issue tracked by multiple existing issues (#1013, #2959, #5007, #5265). Three proposals filed targeting the agents repo and source repo.

Proposals filed

• Review agent should read plan/spec documents linked from PR description and validate implementation completeness (in fullsend-ai/agents)
• Review agent re-review should perform targeted side-effect analysis when fix commits address HIGH/CRITICAL findings (in fullsend-ai/agents)
• Counter-evidence for review autonomy: agent missed all high-impact findings on repos sync/diff feature PR (in fullsend-ai/fullsend)