feat(repos): add repos diff and repos sync CLI commands by ggallen · Pull Request #4079 · fullsend-ai/fullsend

ggallen · 2026-07-10T21:35:23Z

Summary

Add fullsend repos diff command — read-only preview of configuration drift between the repos.yaml manifest and actual forge state for installed repos
Add fullsend repos sync command — writes variables and secrets to reconcile installed repos with the manifest
Sync reconciles 3 variables (FULLSEND_MINT_URL, FULLSEND_GCP_REGION, FULLSEND_PER_REPO_INSTALL) and 1 secret (FULLSEND_GCP_PROJECT_ID); does not touch scaffold shim version or harness files (managed by repos upgrade)
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`	37 tests — 93-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 37 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 96.3%, Sync 93.8%, applyChanges/resolveSecretValue/FormatDiffTable/secretDisplay 100%
CI checks

🤖 Generated with Claude Code

fullsend-ai-review · 2026-07-10T21:36:16Z

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

qodo-code-review · 2026-07-10T21:36:37Z

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

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 · 2026-07-10T21:39:55Z

Codecov Report

❌ Patch coverage is 80.39648% with 178 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/repos.go	65.28%	103 Missing and 39 partials ⚠️
internal/repos/sync.go	89.67%	18 Missing and 4 partials ⚠️
internal/repos/manifest_edit.go	91.80%	5 Missing and 5 partials ⚠️
internal/cli/admin.go	75.00%	1 Missing and 1 partial ⚠️
internal/repos/uninstall.go	98.34%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

qodo-code-review · 2026-07-10T21:42:45Z

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 · 2026-07-10T21:45:39Z

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

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

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 (2)

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 (3)

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 (4)

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 (5)

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 (6)

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:

Add an Entry RepoEntry or Config ResolvedConfig field to the Change struct so applyChanges has the correct per-entry config.
Restructure Sync to call ExpandGlobs + ResolveConfigForEntry itself rather than re-resolving through ResolveConfig.
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 (7)

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 (8)

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 (9)

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:

Add an Entry RepoEntry or Config ResolvedConfig field to the Change struct so applyChanges has the correct per-entry config.
Restructure Sync to call ExpandGlobs + ResolveConfigForEntry itself rather than re-resolving through ResolveConfig.
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 (10)

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 (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:

Add an Entry RepoEntry or Config ResolvedConfig field to the Change struct so applyChanges has the correct per-entry config.
Restructure Sync to call ExpandGlobs + ResolveConfigForEntry itself rather than re-resolving through ResolveConfig.
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:

Add an Entry RepoEntry or Config ResolvedConfig field to the Change struct so applyChanges has the correct per-entry config.
Restructure Sync to call ExpandGlobs + ResolveConfigForEntry itself rather than re-resolving through ResolveConfig.
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

fullsend-ai-review · 2026-07-11T00:55:30Z

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

fullsend-ai-review · 2026-07-11T01:54:33Z

🤖 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 · 2026-07-11T02:17:59Z

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

fullsend-ai-review · 2026-07-11T02:34:39Z

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

fullsend-ai-review · 2026-07-11T02:51:29Z

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

fullsend-ai-review · 2026-07-11T02:59:05Z

🤖 Finished Review · ❌ Failure · Started 2:59 AM UTC · Completed 3:09 AM UTC
Commit: 92ee627 · View workflow run →

Implements ADR-0057 PR 8: three new repos subcommands for managing per-repo installations via the repos.yaml manifest. - repos add: add repo entries to the manifest (with optional --install) - repos remove: remove entries from manifest (with optional --uninstall) - repos uninstall: tear down fullsend from repos without modifying manifest Key fixes from review: - Fix glob-matched repos WIF cleanup via resolveConfigWithGlobs helper - Fix silent WIF provider deletion skip (now returns error) - Add repoNamePattern validation in AddToManifest - Add confirmGlobAction for destructive glob operations - Add manifest.Validate() calls to add/remove/uninstall commands - Add concurrency validation gated on --uninstall flag Also fixes a pre-existing WIF provider leak in batch_install cleanup paths: splitProjectAdapter.DeletePerRepoWIF now deletes the WIF provider from the inference project in addition to deregistering from the mint. BREAKING CHANGE: repos install no longer accepts --repo flag; use positional arguments instead (e.g. `fullsend repos install acme/api` instead of `fullsend repos install --repo acme/api`). Signed-off-by: Greg Allen <gallen@fullsend.ai> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

fullsend-ai-review · 2026-07-11T03:13:01Z

🤖 Finished Review · ✅ Success · Started 3:13 AM UTC · Completed 3:21 AM UTC
Commit: 94e0b1a · View workflow run →

Signed-off-by: Greg Allen <gallen@fullsend.ai> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

fullsend-ai-review · 2026-07-11T10:44:05Z

🤖 Finished Review · ✅ Success · Started 10:44 AM UTC · Completed 10:54 AM UTC
Commit: 3d80470 · View workflow run →

fullsend-ai-review

See the review comment for full details.

ggallen requested a review from a team as a code owner July 10, 2026 21:35

qodo-code-review Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread internal/cli/repos.go Outdated

Comment thread internal/repos/sync.go Outdated

Comment thread internal/cli/repos.go Outdated

Comment thread internal/repos/sync.go Outdated

Comment thread internal/repos/sync.go