feat(#5620): support mixed-forge manifests with per-repo client resolution

[fullsend-ai-coder]

Summary

Introduces ForgeClientFactory interface with lazy per-forge client creation so mixed-forge manifests (GitHub + GitLab repos in a single repos.yaml) route API calls through the correct forge-specific client at runtime. Previously, forgeClientFromManifest created a single client from defaults.forge, causing GitLab entries to silently route through the GitHub API.

Changes

• ForgeClientFactory interface (internal/repos/forge_config.go): ConfigFor(forgeName) (ForgeConfig, error) returns a ForgeConfig with a live Client, forge-specific patterns, and workflow paths
• forgeClientFactory implementation (internal/cli/forge_client.go): lazily creates and caches per-forge clients with sync.Mutex for concurrent safety; at most 2 clients per command (GitHub + GitLab)
• ForgeConfig.Client field: each ForgeConfig now carries its own API client, eliminating the need to pass client and fc as separate parameters
• ResolvedConfig.ForgeConfig field: makes ResolvedConfig the single per-repo object with owner, repo, client, and patterns
• All public batch functions updated: Upgrade, Status, BatchInstall, Diff, Sync, Uninstall, Init, AddToManifest, ExpandGlobs now accept ForgeClientFactory instead of forge.Client
• Per-repo function simplification: upgradeRepo, checkRepoStatus etc. take ResolvedConfig instead of 5-9 separate parameters
• Mixed-forge-same-owner validation: Manifest.Validate() rejects manifests where the same owner has entries with different forges (a GitHub org and GitLab group with the same name are different entities)
• Doc update: removed the "mixed-forge manifests not yet supported" caveat from docs/guides/getting-started/repo-management.md

Testing

• All existing internal/repos/... tests pass (6.2s, with -race)
• All existing internal/cli/... tests pass for repos commands
• go build ./... — full project compiles cleanly
• go vet ./internal/repos/... ./internal/cli/... — no issues
• Secret scan passes on all changed files
• Test helper newTestClientFactory wraps FakeClient for backward-compatible test patterns

---

Closes #5620

Post-script verification

• Branch is not main/master (agent/5620-mixed-forge-clients)
• Secret scan passed (gitleaks — 896bb57d9f55d9aa6567e537fd92022e65e430e1..HEAD)
• PR body secret scan passed (gitleaks — no-git)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:50 AM UTC · Completed 2:04 AM UTC
Commit: 2459a59 · View workflow run →

[github-actions]

Site preview

Preview: https://42fd622b-site.fullsend-ai.workers.dev

Commit: db813e5aaeca7a2ec745b079e8c949b675349680

[codecov]

Codecov Report

❌ Patch coverage is 77.37557% with 50 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/repos.go	69.69%	8 Missing and 12 partials ⚠️
internal/repos/status.go	62.50%	5 Missing and 1 partial ⚠️
internal/repos/sync.go	78.57%	4 Missing and 2 partials ⚠️
internal/repos/init.go	60.00%	2 Missing and 2 partials ⚠️
internal/repos/batch_install.go	75.00%	2 Missing and 1 partial ⚠️
internal/repos/manifest_edit.go	57.14%	2 Missing and 1 partial ⚠️
internal/repos/uninstall.go	75.00%	2 Missing and 1 partial ⚠️
internal/repos/upgrade.go	72.72%	2 Missing and 1 partial ⚠️
internal/repos/manifest.go	94.11%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [logic-error] internal/cli/repos.go — checkAllForgeScopes iterates DistinctForges() computed from the entire manifest before any --repo filtering. In a mixed-forge manifest, running e.g. repos diff --repo gitlab-group/repo still forces scope checking to create a GitHub client (and thus requires a valid GH_TOKEN), even though no GitHub repos will be touched. See also: [fail-open] finding at this location.
  Remediation: Pass the post-filter repo list (or the filtered set of forges) to checkAllForgeScopes instead of the full manifest.

• [fail-open] internal/cli/repos.go — checkAllForgeScopes skips scope checking for all non-GitHub forges (forgeName != repos.ForgeGitHub). For GitLab-only manifests, no token scope validation occurs before destructive operations. Intentional because GitLab does not support scope introspection, but misscoped GitLab tokens fail later with unclear per-repo errors. See also: [logic-error] finding at this location.

• [behavior-change] internal/cli/repos_test.go — TestReposDiffCmd_GitLabNoToken_WithRepos asserts require.NoError when a manifest has forge: gitlab, actual repos, and no GITLAB_TOKEN. The diff command exits 0 because per-repo forge client errors are surfaced as warnings rather than fatal errors. While this is the intended design, CI pipelines consuming the exit code would see success even though all repos failed to be examined.

• [lazy-credential-validation] internal/cli/forge_client.go — The ForgeClientFactory lazily creates forge clients on first use. Previously, forgeClientFromManifest eagerly validated that a GitLab token existed before any operations began. With lazy creation, a missing GITLAB_TOKEN is only detected when the first per-repo operation requests a GitLab client. The repos init path still eagerly validates via clients.ConfigFor(cfg.forge), so the primary onboarding path is protected. Intentional behavior change.

• [edge-case] internal/repos/batch_install.go — Phase 2 (TOCTOU re-check) accesses d.resolved.ForgeConfig.Client stored during Phase 1. ForgeConfig is embedded by value in ResolvedConfig, so the Client field (an interface pointer) captured during Phase 1 remains valid because the factory caches and reuses client instances. Correct by design but relies on the factory's caching invariant.

Previous run

Review

Findings

Low

• [logic-error] internal/cli/repos.go — checkAllForgeScopes iterates DistinctForges() computed from the entire manifest before any --repo filtering. In a mixed-forge manifest, running e.g. repos diff --repo gitlab-group/repo still forces scope checking to create a GitHub client (and thus requires a valid GH_TOKEN), even though no GitHub repos will be touched. See also: [fail-open] finding at this location.
  Remediation: Pass the post-filter repo list (or the filtered set of forges) to checkAllForgeScopes instead of the full manifest.

• [fail-open] internal/cli/repos.go — checkAllForgeScopes skips scope checking for all non-GitHub forges (forgeName != repos.ForgeGitHub). For GitLab-only manifests, no token scope validation occurs before destructive operations. Intentional because GitLab does not support scope introspection, but misscoped GitLab tokens fail later with unclear per-repo errors. See also: [logic-error] finding at this location.

• [behavior-change] internal/cli/repos_test.go — TestReposDiffCmd_GitLabNoToken_WithRepos asserts require.NoError when a manifest has forge: gitlab, actual repos, and no GITLAB_TOKEN. The diff command exits 0 because per-repo forge client errors are surfaced as warnings rather than fatal errors. While this is the intended design, CI pipelines consuming the exit code would see success even though all repos failed to be examined.

• [lazy-credential-validation] internal/cli/forge_client.go — The ForgeClientFactory lazily creates forge clients on first use. Previously, forgeClientFromManifest eagerly validated that a GitLab token existed before any operations began. With lazy creation, a missing GITLAB_TOKEN is only detected when the first per-repo operation requests a GitLab client. The repos init path still eagerly validates via clients.ConfigFor(cfg.forge), so the primary onboarding path is protected. Intentional behavior change.

• [edge-case] internal/repos/batch_install.go — Phase 2 (TOCTOU re-check) accesses d.resolved.ForgeConfig.Client stored during Phase 1. ForgeConfig is embedded by value in ResolvedConfig, so the Client field (an interface pointer) captured during Phase 1 remains valid because the factory caches and reuses client instances. Correct by design but relies on the factory's caching invariant.

---

Prior review findings addressed in this revision:

• [doc-code-mismatch] (low → resolved): docs/plans/repos-management.md Install function signature correctly shows forge.Client, matching the implementation where BatchInstall pre-resolves the client.
• [stale-signature] (low → resolved): docs/plans/repos-init.md Init function signature updated to ForgeClientFactory.

Previous run (2)

Review

Findings

Low

• [logic-error] internal/cli/repos.go — checkAllForgeScopes iterates DistinctForges() computed from the entire manifest before any --repo filtering. In a mixed-forge manifest, running e.g. repos diff --repo gitlab-group/repo still forces scope checking to create a GitHub client (and thus requires a valid GH_TOKEN), even though no GitHub repos will be touched. See also: [fail-open] finding at this location.
  Remediation: Pass the post-filter repo list (or the filtered set of forges) to checkAllForgeScopes instead of the full manifest.

• [fail-open] internal/cli/repos.go — checkAllForgeScopes skips scope checking for all non-GitHub forges (forgeName != repos.ForgeGitHub). For GitLab-only manifests, no token scope validation occurs before destructive operations. Intentional because GitLab does not support scope introspection, but misscoped GitLab tokens fail later with unclear per-repo errors. Zero-repo manifests also skip scope checking entirely because DistinctForges() returns empty when Repos is empty. See also: [logic-error] finding at this location.

• [behavior-change] internal/cli/repos_test.go — TestReposDiffCmd_GitLabNoToken_WithRepos asserts require.NoError when a manifest has forge: gitlab, actual repos, and no GITLAB_TOKEN. The diff command exits 0 because per-repo forge client errors are surfaced as warnings rather than fatal errors. While this is the intended design, CI pipelines consuming the exit code would see success even though all repos failed to be examined.

• [edge-case] internal/repos/batch_install.go — Phase 2 (TOCTOU re-check) accesses d.resolved.ForgeConfig.Client stored during Phase 1. ForgeConfig is embedded by value in ResolvedConfig, so the Client field (an interface pointer) captured during Phase 1 remains valid because the factory caches and reuses client instances. Correct by design but relies on the factory's caching invariant.

---

Prior review findings addressed in this revision:

• [doc-code-mismatch] (low → resolved): docs/plans/repos-management.md Install function signature correctly shows forge.Client, matching the implementation where BatchInstall pre-resolves the client.
• [stale-signature] (low → resolved): docs/plans/repos-init.md Init function signature updated to ForgeClientFactory.

Previous run (3)

Review

Findings

Low

• [doc-code-mismatch] docs/plans/repos-management.md — The plan document updates the Install function signature from client forge.Client to clients ForgeClientFactory, but the actual implementation in internal/repos/install.go:162 still takes forge.Client. BatchInstall calls Install with dr.resolved.ForgeConfig.Client (a single pre-resolved client), not a factory.
  Remediation: Either revert the plan-doc signature change for Install (since the actual code still accepts forge.Client), or add a note that Install receives a pre-resolved client from BatchInstall.

• [logic-error] internal/cli/repos.go — checkAllForgeScopes runs before repo filtering and checks GitHub scope even when --repo selects only GitLab repos. In a mixed-forge manifest, this requires a valid GitHub token for commands targeting only GitLab repos. See also: [fail-open] finding at this location.
  Remediation: Pass the repo filter into checkAllForgeScopes so it only checks forges actually needed, or defer scope checking to per-repo goroutines.

• [fail-open] internal/cli/repos.go — checkAllForgeScopes skips scope checking for all non-GitHub forges (forgeName != repos.ForgeGitHub). For GitLab-only manifests, no token scope validation occurs before destructive operations. Intentional because GitLab does not support scope introspection, but misscoped GitLab tokens fail later with unclear per-repo errors. See also: [logic-error] finding at this location.

• [behavior-change] internal/cli/repos_test.go — TestReposDiffCmd_GitLabNoToken_WithRepos asserts require.NoError when a manifest has forge: gitlab, actual repos, and no GITLAB_TOKEN. The diff command exits 0 with a warning. While the warning is visible, exit code 0 for "all repos failed credential resolution" could mislead CI pipelines.

• [edge-case] internal/repos/batch_install.go — Phase 2 (TOCTOU re-check) accesses d.resolved.ForgeConfig.Client set during Phase 1. The ForgeConfig is stored by value in ResolvedConfig, capturing the cached client interface pointer. Correct by design but assumes factory caching behavior.

• [stale-signature] docs/plans/repos-init.md — The Init function signature shows the old parameter client forge.Client. This PR changed it to clients ForgeClientFactory.
  Remediation: Update the signature from client forge.Client to clients ForgeClientFactory.

---

Prior review findings addressed in this revision:

• [stale-signature] (low → resolved): Six function signatures in docs/plans/repos-management.md (ExpandGlobs, Status, BatchInstall, Diff, Sync, Upgrade) were updated from client forge.Client to clients ForgeClientFactory. However, the Install signature was also updated when the code was not changed — see [doc-code-mismatch] above.
• [misleading-behavior] (low → resolved): The doc comment on checkAllForgeScopes now correctly says "GitHub forges" rather than "every distinct forge."
• [error-handling] (low → resolved): AddToManifest best-effort probing behavior is unchanged and intentional; not resurfaced.
• [fail-open] (low → info, suppressed): ResolveConfigWithGlobs ignoring matchesPattern errors is mitigated by Validate() catching invalid patterns upstream. Downgraded to info.
• [naming-inconsistency] (low → info, suppressed): Variable naming inconsistency for ForgeClientFactory instances is cosmetic. Downgraded to info.
• [doc-comment-accuracy] (low → info, suppressed): Doc comment ecosystem around forgeClientFactory not resurfaced.

Previous run (4)

Review

Findings

Low

• [logic-error] internal/cli/repos.go — checkAllForgeScopes runs before repo filtering and checks GitHub scope even when --repo selects only GitLab repos. In a mixed-forge manifest, this requires a valid GitHub token for commands targeting only GitLab repos. See also: [scope-check-gap] finding at this location.
  Remediation: Pass the repo filter into checkAllForgeScopes so it only checks forges actually needed, or defer scope checking to per-repo goroutines.

• [scope-check-gap] internal/cli/repos.go — checkAllForgeScopes explicitly skips non-GitHub forges (if forgeName != "" && forgeName != repos.ForgeGitHub { continue }). GitLab repos proceed through batch operations with no upfront permission validation — errors surface only at per-repo operation time via clients.ConfigFor. The old code also only checked the default forge, so this is not a regression, but the lazy factory makes it more visible. See also: [logic-error] finding at this location.
  Remediation: At minimum, eagerly validate that ConfigFor succeeds for all distinct forges before starting batch operations.

• [stale-signature] docs/plans/repos-management.md — Six function signatures in the implementation plan doc still show the old client forge.Client parameter (ExpandGlobs, Status, BatchInstall, Diff, Sync, Upgrade). This PR replaced them with clients ForgeClientFactory.
  Remediation: Update the plan doc signatures or add a note about the ForgeClientFactory pattern.

• [naming-inconsistency] internal/cli/repos.go — Local variable naming for ForgeClientFactory instances is inconsistent: some functions use prefixed names (diffClients, syncClients, upgradeClients, uninstallClients) while others use clients. The prefixes add no disambiguation since each variable is function-scoped.

• [misleading-behavior] internal/cli/repos.go — The doc comment on checkAllForgeScopes says "validates token permissions for every distinct forge used in the manifest" but the implementation skips all non-GitHub forges.
  Remediation: Update the doc comment to say "validates token permissions for GitHub forges used in the manifest" or explain why non-GitHub forges are skipped.

• [behavior-change] internal/cli/repos_test.go — Several CLI tests changed from asserting require.Error to require.NoError for missing GitLab token scenarios. This correctly reflects the new lazy-client semantics, but all changed tests use zero-repo manifests. No test covers a GitLab manifest with actual repos and no token.
  Remediation: Add a test that verifies a GitLab manifest with repos fails appropriately during batch operations when no token is set.

• [error-handling] internal/repos/manifest_edit.go — When clients.ConfigFor(entryForge) returns an error in AddToManifest, the code logs via the progress callback and continues. This is intentional best-effort probing, but a user adding a GitLab repo without a GitLab token will silently proceed with no probe.

• [edge-case] internal/repos/batch_install.go — Phase 2 (TOCTOU re-check) accesses d.resolved.ForgeConfig.Client set during Phase 1. The ForgeConfig is stored by value in ResolvedConfig, capturing the cached client interface pointer. Correct by design but assumes factory caching behavior.

• [fail-open] internal/repos/manifest.go — ResolveConfigWithGlobs ignores errors from matchesPattern via if ok, _ := matchesPattern(...). A malformed glob pattern silently fails to match. Mitigated by Validate() catching invalid patterns upstream.

• [doc-comment-accuracy] internal/cli/forge_client.go — The doc comment ecosystem around forgeClientFactory and newForgeClientFactory is slightly confusing: the type comment references the concrete struct while the constructor comment references the ForgeClientFactory interface return type.

---

Prior review findings addressed in this revision:

• [logic-error] (medium → low): DistinctForges() now only includes forges from actual repo entries (not defaults.forge), and checkAllForgeScopes skips non-GitHub forges. The zero-repos and GitLab-misleading-message issues are resolved. Remaining aspect (not filter-aware) downgraded to low.
• [misleading-behavior] (low → resolved): checkAllForgeScopes now skips non-GitHub forges entirely, eliminating the misleading "fine-grained token detected" message for GitLab.
• [behavior-change] (low → resolved): Asymmetry between commands reduced by the GitLab skip in checkAllForgeScopes.
• [error-handling] (low → unchanged): AddToManifest best-effort probing behavior preserved.

Previous run

Review

Findings

Medium

• [logic-error] internal/cli/repos.go — checkAllForgeScopes eagerly creates API clients for every distinct forge in the manifest (via DistinctForges()), including defaults.forge even when the manifest has zero repos. This means commands like repos diff, repos sync, repos upgrade, and repos uninstall will fail with "no GitLab token found" on a GitLab-default manifest with zero repos, while repos status (which does not call checkAllForgeScopes) will succeed. Additionally, using --repo to filter to only GitHub repos in a mixed-forge manifest will still fail if the GitLab token is missing, because scope checking is done for ALL forges regardless of the repo filter.
  Remediation: Either (a) pass the repo filter into checkAllForgeScopes so it only checks forges that will actually be used, or (b) move checkAllForgeScopes to run after glob expansion so it only checks forges of resolved repos, or (c) make it consistent by also calling checkAllForgeScopes in runReposStatus.

Low

• [behavior-change] internal/cli/repos.go — The lazy factory design relaxes the upfront token requirement for several paths: repos add now succeeds on a GitLab manifest without GITLAB_TOKEN (the probe logs a warning via the progress callback and continues), and repos status succeeds on an empty GitLab manifest without a token since no API calls are made. Other commands (repos diff, repos sync, repos upgrade, repos install, repos uninstall) still fail eagerly via checkAllForgeScopes. This asymmetry is a consequence of the lazy client factory design.

• [misleading-behavior] internal/cli/repos.go — checkAllForgeScopes calls checkPerRepoScopes for every distinct forge, including GitLab. The GitLab client's GetTokenScopes returns nil, causing checkPerRepoScopes to print "Preflight skipped: fine-grained token detected (scopes cannot be verified)". This is misleading — it's not a fine-grained token, it's a GitLab PAT. This message will appear in mixed-forge manifests but was never reachable before this PR.
  Remediation: Either skip scope checking for GitLab forges in checkAllForgeScopes, or add a forge-aware message that says "forge does not support scope introspection" instead of the GitHub-specific "fine-grained token detected" message.

• [error-handling] internal/repos/manifest_edit.go:71 — When clients.ConfigFor(entryForge) returns an error in AddToManifest, the code logs via the progress callback and continues. This is intentional best-effort behavior for probing, but forge client errors indicate configuration or authentication problems rather than transient failures. A user adding a GitLab repo without a GitLab token will silently proceed with no probe.

---

Prior review findings addressed in this revision:

• [logic-error] (medium → resolved): scaffoldCommitFn now uses manifest.ResolveConfigWithGlobs(owner, repo) instead of manifest.ResolveConfig(owner, repo), correctly handling glob-matched repos.
• [logic-error] (medium → resolved): commitFn in runReposUpgrade now also uses m.ResolveConfigWithGlobs(owner, repo), fixing the same glob-expansion bug.

Previous run (5)

Review

Findings

Medium

• [logic-error] internal/cli/repos.go:421 — scaffoldCommitFn in runReposInstall calls manifest.ResolveConfig(owner, repo) which performs exact string matching against m.Repos[].Repo. For repos matched via glob patterns (e.g., manifest entry acme/* expanded to acme/api), the manifest still contains the original glob pattern, so exact match fails and the function returns "repo acme/api not found in manifest". The old code captured a single client variable and did not need this lookup. The existing resolveConfigWithGlobs helper in uninstall.go handles glob fallback correctly but is not used here. This is a regression for glob-pattern manifests on the install path.
  Remediation: Replace manifest.ResolveConfig with resolveConfigWithGlobs, or refactor the commit callback to receive the pre-resolved ForgeConfig from the BatchInstall goroutine (which already has the correct per-repo config cached in dr.resolved.ForgeConfig).

• [logic-error] internal/cli/repos.go:1340 — commitFn in runReposUpgrade has the same glob-expansion bug: m.ResolveConfig(owner, repo) does exact matching and will fail for any repo matched via a glob pattern. The Upgrade function dispatches commitFn with concrete owner/repo names after glob expansion, so every glob-matched repo hits the error path.
  Remediation: Same fix as scaffoldCommitFn — use resolveConfigWithGlobs, or propagate the already-resolved ForgeConfig from the Upgrade goroutine through the closure.

Low

• [behavior-change] internal/cli/repos.go — The lazy factory design means operations on GitLab manifests with missing GITLAB_TOKEN no longer fail eagerly during dry-run or empty-manifest scenarios (confirmed by test changes in TestReposStatus_GitLabNoToken and TestReposAddCmd_GitLabNoToken). This is generally an improvement (dry-run shouldn't require credentials), but repos add now silently skips the installation-state probe when the forge client can't be created, potentially adding repos without detecting existing installations.

• [error-handling] internal/repos/manifest_edit.go:71 — When clients.ConfigFor(entryForge) returns an error in AddToManifest, the code logs via the progress callback and continues. While probing is best-effort, forge client errors specifically indicate configuration or authentication problems (wrong token, unsupported forge) rather than transient failures. These should be distinguished from probe failures so users know the probe was skipped due to misconfiguration.

---

Prior review findings addressed in this revision:

• [edge-case] (low → resolved): runReposRemove now always initializes uninstallClients via newForgeClientFactory in the else branch — the !opts.dryRun guard was removed, eliminating the nil factory risk.
• [authorization-bypass] (medium → resolved): checkAllForgeScopes now iterates over m.DistinctForges() and validates token permissions for every distinct forge.
• [fail-open] (low → resolved): Both scaffoldCommitFn and commitFn now check the boolean return value from manifest.ResolveConfig() and return a clear error when the repo is not found.
• [test-adequacy] (low → resolved): TestValidate_RejectsSameOwnerMixedForge and TestValidate_AllowsDifferentOwnersDifferentForges now cover the mixed-forge-same-owner validation.
• [missing-documentation] (low → resolved): The same-owner-same-forge constraint is now documented in repo-management.md.

Previous run (6)

Review

Findings

Low

• [edge-case] internal/cli/repos.go — In runReposRemove, when opts.uninstall is true and opts.dryRun is true and opts.testClient is nil, uninstallClients is left as nil (the else if !opts.dryRun branch is skipped). This nil value is passed to Uninstall(), which returns early for dry-run before calling clients.ConfigFor(). The code is currently safe, but the nil factory is an implicit contract — if Uninstall's dry-run path ever changes to validate the factory or call ConfigFor, it would panic. Notably, runReposUninstall does NOT have this guard and always initializes the factory, suggesting the guard in runReposRemove was carried forward from the old single-client code.
  Remediation: Consider initializing the factory unconditionally (removing the !opts.dryRun guard) for defensive safety, or adding a nil-check comment documenting the invariant.

---

Prior review findings addressed in this revision:

• [authorization-bypass] (medium → resolved): checkAllForgeScopes now iterates over m.DistinctForges() and validates token permissions for every distinct forge, fixing the gap where only the default forge's scopes were checked.
• [fail-open] (low → resolved): Both scaffoldCommitFn and commitFn now check the boolean return value from manifest.ResolveConfig() and return a clear error when the repo is not found.
• [test-integrity] (low → resolved): The contextAwareClient override IS preserved through the newTestClientFactory wrapper; the prior finding was incorrect.
• [test-adequacy] (low → resolved): TestValidate_RejectsSameOwnerMixedForge and TestValidate_AllowsDifferentOwnersDifferentForges now cover the mixed-forge-same-owner validation.
• [missing-documentation] (low → resolved): The same-owner-same-forge constraint is now documented in repo-management.md.

Previous run

Review

Findings

Medium

• [authorization-bypass] internal/cli/repos.go:380 — checkPerRepoScopes only runs against the manifest's default forge client (manifest.Defaults.Forge). In a mixed-forge manifest where repos use different forges, the scope check is skipped for the non-default forge. For example, if defaults.forge is github and some repos override to gitlab, the GitLab token's permissions are never validated by checkPerRepoScopes before performing install, uninstall, diff, sync, or upgrade operations on those GitLab repos. This was a pre-existing limitation (scope check only ran on the single client), but this PR makes it architecturally explicit and newly reachable in production by enabling mixed-forge manifests.
  Remediation: Run checkPerRepoScopes (or an equivalent forge-specific scope check) against every distinct forge client that the manifest will use. After expanding globs or walking entries, collect the set of distinct forge names and validate each.

Low

• [test-integrity] internal/repos/batch_install_test.go:1221 — TestBatchInstall_ContextCancellation_Phase1 previously passed a contextAwareClient (which overrides GetRepoVariable to check ctx.Err()) to BatchInstall. The PR now extracts client.FakeClient and wraps it in newTestClientFactory, discarding the context-aware override. The test still validates the primary cancellation path via the semaphore select statement, but it no longer covers the race window where a goroutine passes the semaphore before the cancellation is detected.
  Remediation: Create a contextAwareTestClientFactory that preserves the GetRepoVariable override, or restructure the test to verify cancellation at the factory.ConfigFor level.

• [missing-documentation] docs/guides/getting-started/repo-management.md:83 — The PR adds validation that rejects manifests where the same owner has repos with different forges (manifest.go lines 671–684), but this constraint is not documented in the Multi-forge manifests section. The clear validation error message mitigates this, but users would benefit from knowing the constraint before encountering it.
  Remediation: Add a note explaining that all repos under the same owner must use the same forge.

• [fail-open] internal/cli/repos.go:425 — In the scaffoldCommitFn closure (install) and commitFn closure (upgrade), the second return value from manifest.ResolveConfig(owner, repo) is discarded (rc, _ := ...). For glob-matched repos not listed explicitly in the manifest, the returned ResolvedConfig has an empty Forge field, which falls through to GitHub via ForgeConfigFor's default case. If the repo was actually a GitLab repo matched via glob, the commit function would use the wrong client. The same-owner-same-forge validation mitigates this in practice.
  Remediation: Pass the ResolvedConfig (including resolved forge) from the batch caller rather than re-resolving inside the commit closure, or check the ok return value.

• [test-adequacy] internal/repos/manifest.go:430 — The new mixed-forge-same-owner validation rule (Validate() rejects manifests where the same owner has entries with different forges) has no dedicated unit test. While existing tests exercise Validate(), none specifically cover this new constraint — a regression would go undetected.
  Remediation: Add a test in manifest_test.go that creates a manifest with two entries for the same owner but different forges and asserts that Validate returns an error containing "all repos under the same owner must use the same forge".

---

Labels: PR modifies repos management code (install, status, sync, upgrade, init, uninstall) and forge client abstraction layer

Previous run (7)

Review

Findings

Low

• [edge-case] internal/cli/repos.go — In runReposRemove, when opts.uninstall is true and opts.dryRun is true and opts.testClient is nil, uninstallClients is left as nil (the else if !opts.dryRun branch is skipped). This nil value is passed to Uninstall(), which returns early for dry-run before calling clients.ConfigFor(). The code is currently safe, but the nil factory is an implicit contract — if Uninstall's dry-run path ever changes to validate the factory or call ConfigFor, it would panic. Notably, runReposUninstall does NOT have this guard and always initializes the factory, suggesting the guard in runReposRemove was carried forward from the old single-client code.
  Remediation: Consider initializing the factory unconditionally (removing the !opts.dryRun guard) for defensive safety, or adding a nil-check comment documenting the invariant.

---

Prior review findings addressed in this revision:

• [authorization-bypass] (medium → resolved): checkAllForgeScopes now iterates over m.DistinctForges() and validates token permissions for every distinct forge, fixing the gap where only the default forge's scopes were checked.
• [fail-open] (low → resolved): Both scaffoldCommitFn and commitFn now check the boolean return value from manifest.ResolveConfig() and return a clear error when the repo is not found.
• [test-integrity] (low → resolved): The contextAwareClient override IS preserved through the newTestClientFactory wrapper; the prior finding was incorrect.
• [test-adequacy] (low → resolved): TestValidate_RejectsSameOwnerMixedForge and TestValidate_AllowsDifferentOwnersDifferentForges now cover the mixed-forge-same-owner validation.
• [missing-documentation] (low → resolved): The same-owner-same-forge constraint is now documented in repo-management.md.

Previous run (8)

Review

Findings

Medium

• [authorization-bypass] internal/cli/repos.go:380 — checkPerRepoScopes only runs against the manifest's default forge client (manifest.Defaults.Forge). In a mixed-forge manifest where repos use different forges, the scope check is skipped for the non-default forge. For example, if defaults.forge is github and some repos override to gitlab, the GitLab token's permissions are never validated by checkPerRepoScopes before performing install, uninstall, diff, sync, or upgrade operations on those GitLab repos. This was a pre-existing limitation (scope check only ran on the single client), but this PR makes it architecturally explicit and newly reachable in production by enabling mixed-forge manifests.
  Remediation: Run checkPerRepoScopes (or an equivalent forge-specific scope check) against every distinct forge client that the manifest will use. After expanding globs or walking entries, collect the set of distinct forge names and validate each.

Low

• [test-integrity] internal/repos/batch_install_test.go:1221 — TestBatchInstall_ContextCancellation_Phase1 previously passed a contextAwareClient (which overrides GetRepoVariable to check ctx.Err()) to BatchInstall. The PR now extracts client.FakeClient and wraps it in newTestClientFactory, discarding the context-aware override. The test still validates the primary cancellation path via the semaphore select statement, but it no longer covers the race window where a goroutine passes the semaphore before the cancellation is detected.
  Remediation: Create a contextAwareTestClientFactory that preserves the GetRepoVariable override, or restructure the test to verify cancellation at the factory.ConfigFor level.

• [missing-documentation] docs/guides/getting-started/repo-management.md:83 — The PR adds validation that rejects manifests where the same owner has repos with different forges (manifest.go lines 671–684), but this constraint is not documented in the Multi-forge manifests section. The clear validation error message mitigates this, but users would benefit from knowing the constraint before encountering it.
  Remediation: Add a note explaining that all repos under the same owner must use the same forge.

• [fail-open] internal/cli/repos.go:425 — In the scaffoldCommitFn closure (install) and commitFn closure (upgrade), the second return value from manifest.ResolveConfig(owner, repo) is discarded (rc, _ := ...). For glob-matched repos not listed explicitly in the manifest, the returned ResolvedConfig has an empty Forge field, which falls through to GitHub via ForgeConfigFor's default case. If the repo was actually a GitLab repo matched via glob, the commit function would use the wrong client. The same-owner-same-forge validation mitigates this in practice.
  Remediation: Pass the ResolvedConfig (including resolved forge) from the batch caller rather than re-resolving inside the commit closure, or check the ok return value.

• [test-adequacy] internal/repos/manifest.go:430 — The new mixed-forge-same-owner validation rule (Validate() rejects manifests where the same owner has entries with different forges) has no dedicated unit test. While existing tests exercise Validate(), none specifically cover this new constraint — a regression would go undetected.
  Remediation: Add a test in manifest_test.go that creates a manifest with two entries for the same owner but different forges and asserts that Validate returns an error containing "all repos under the same owner must use the same forge".

---

Labels: PR modifies repos management code (install, status, sync, upgrade, init, uninstall) and forge client abstraction layer

Previous run (9)

Review

Findings

Low

• [logic-error] internal/cli/repos.go — checkAllForgeScopes runs before repo filtering and checks GitHub scope even when --repo selects only GitLab repos. In a mixed-forge manifest, this requires a valid GitHub token for commands targeting only GitLab repos. See also: [scope-check-gap] finding at this location.
  Remediation: Pass the repo filter into checkAllForgeScopes so it only checks forges actually needed, or defer scope checking to per-repo goroutines.

• [scope-check-gap] internal/cli/repos.go — checkAllForgeScopes explicitly skips non-GitHub forges (if forgeName != "" && forgeName != repos.ForgeGitHub { continue }). GitLab repos proceed through batch operations with no upfront permission validation — errors surface only at per-repo operation time via clients.ConfigFor. The old code also only checked the default forge, so this is not a regression, but the lazy factory makes it more visible. See also: [logic-error] finding at this location.
  Remediation: At minimum, eagerly validate that ConfigFor succeeds for all distinct forges before starting batch operations.

• [stale-signature] docs/plans/repos-management.md — Six function signatures in the implementation plan doc still show the old client forge.Client parameter (ExpandGlobs, Status, BatchInstall, Diff, Sync, Upgrade). This PR replaced them with clients ForgeClientFactory.
  Remediation: Update the plan doc signatures or add a note about the ForgeClientFactory pattern.

• [naming-inconsistency] internal/cli/repos.go — Local variable naming for ForgeClientFactory instances is inconsistent: some functions use prefixed names (diffClients, syncClients, upgradeClients, uninstallClients) while others use clients. The prefixes add no disambiguation since each variable is function-scoped.

• [misleading-behavior] internal/cli/repos.go — The doc comment on checkAllForgeScopes says "validates token permissions for every distinct forge used in the manifest" but the implementation skips all non-GitHub forges.
  Remediation: Update the doc comment to say "validates token permissions for GitHub forges used in the manifest" or explain why non-GitHub forges are skipped.

• [behavior-change] internal/cli/repos_test.go — Several CLI tests changed from asserting require.Error to require.NoError for missing GitLab token scenarios. This correctly reflects the new lazy-client semantics, but all changed tests use zero-repo manifests. No test covers a GitLab manifest with actual repos and no token.
  Remediation: Add a test that verifies a GitLab manifest with repos fails appropriately during batch operations when no token is set.

• [error-handling] internal/repos/manifest_edit.go — When clients.ConfigFor(entryForge) returns an error in AddToManifest, the code logs via the progress callback and continues. This is intentional best-effort probing, but a user adding a GitLab repo without a GitLab token will silently proceed with no probe.

• [edge-case] internal/repos/batch_install.go — Phase 2 (TOCTOU re-check) accesses d.resolved.ForgeConfig.Client set during Phase 1. The ForgeConfig is stored by value in ResolvedConfig, capturing the cached client interface pointer. Correct by design but assumes factory caching behavior.

• [fail-open] internal/repos/manifest.go — ResolveConfigWithGlobs ignores errors from matchesPattern via if ok, _ := matchesPattern(...). A malformed glob pattern silently fails to match. Mitigated by Validate() catching invalid patterns upstream.

• [doc-comment-accuracy] internal/cli/forge_client.go — The doc comment ecosystem around forgeClientFactory and newForgeClientFactory is slightly confusing: the type comment references the concrete struct while the constructor comment references the ForgeClientFactory interface return type.

---

Prior review findings addressed in this revision:

• [logic-error] (medium → low): DistinctForges() now only includes forges from actual repo entries (not defaults.forge), and checkAllForgeScopes skips non-GitHub forges. The zero-repos and GitLab-misleading-message issues are resolved. Remaining aspect (not filter-aware) downgraded to low.
• [misleading-behavior] (low → resolved): checkAllForgeScopes now skips non-GitHub forges entirely, eliminating the misleading "fine-grained token detected" message for GitLab.
• [behavior-change] (low → resolved): Asymmetry between commands reduced by the GitLab skip in checkAllForgeScopes.
• [error-handling] (low → unchanged): AddToManifest best-effort probing behavior preserved.

Previous run (10)

Review

Findings

Medium

• [logic-error] internal/cli/repos.go — checkAllForgeScopes eagerly creates API clients for every distinct forge in the manifest (via DistinctForges()), including defaults.forge even when the manifest has zero repos. This means commands like repos diff, repos sync, repos upgrade, and repos uninstall will fail with "no GitLab token found" on a GitLab-default manifest with zero repos, while repos status (which does not call checkAllForgeScopes) will succeed. Additionally, using --repo to filter to only GitHub repos in a mixed-forge manifest will still fail if the GitLab token is missing, because scope checking is done for ALL forges regardless of the repo filter.
  Remediation: Either (a) pass the repo filter into checkAllForgeScopes so it only checks forges that will actually be used, or (b) move checkAllForgeScopes to run after glob expansion so it only checks forges of resolved repos, or (c) make it consistent by also calling checkAllForgeScopes in runReposStatus.

Low

• [behavior-change] internal/cli/repos.go — The lazy factory design relaxes the upfront token requirement for several paths: repos add now succeeds on a GitLab manifest without GITLAB_TOKEN (the probe logs a warning via the progress callback and continues), and repos status succeeds on an empty GitLab manifest without a token since no API calls are made. Other commands (repos diff, repos sync, repos upgrade, repos install, repos uninstall) still fail eagerly via checkAllForgeScopes. This asymmetry is a consequence of the lazy client factory design.

• [misleading-behavior] internal/cli/repos.go — checkAllForgeScopes calls checkPerRepoScopes for every distinct forge, including GitLab. The GitLab client's GetTokenScopes returns nil, causing checkPerRepoScopes to print "Preflight skipped: fine-grained token detected (scopes cannot be verified)". This is misleading — it's not a fine-grained token, it's a GitLab PAT. This message will appear in mixed-forge manifests but was never reachable before this PR.
  Remediation: Either skip scope checking for GitLab forges in checkAllForgeScopes, or add a forge-aware message that says "forge does not support scope introspection" instead of the GitHub-specific "fine-grained token detected" message.

• [error-handling] internal/repos/manifest_edit.go:71 — When clients.ConfigFor(entryForge) returns an error in AddToManifest, the code logs via the progress callback and continues. This is intentional best-effort behavior for probing, but forge client errors indicate configuration or authentication problems rather than transient failures. A user adding a GitLab repo without a GitLab token will silently proceed with no probe.

---

Prior review findings addressed in this revision:

• [logic-error] (medium → resolved): scaffoldCommitFn now uses manifest.ResolveConfigWithGlobs(owner, repo) instead of manifest.ResolveConfig(owner, repo), correctly handling glob-matched repos.
• [logic-error] (medium → resolved): commitFn in runReposUpgrade now also uses m.ResolveConfigWithGlobs(owner, repo), fixing the same glob-expansion bug.

Previous run (11)

Review

Findings

Medium

• [logic-error] internal/cli/repos.go:421 — scaffoldCommitFn in runReposInstall calls manifest.ResolveConfig(owner, repo) which performs exact string matching against m.Repos[].Repo. For repos matched via glob patterns (e.g., manifest entry acme/* expanded to acme/api), the manifest still contains the original glob pattern, so exact match fails and the function returns "repo acme/api not found in manifest". The old code captured a single client variable and did not need this lookup. The existing resolveConfigWithGlobs helper in uninstall.go handles glob fallback correctly but is not used here. This is a regression for glob-pattern manifests on the install path.
  Remediation: Replace manifest.ResolveConfig with resolveConfigWithGlobs, or refactor the commit callback to receive the pre-resolved ForgeConfig from the BatchInstall goroutine (which already has the correct per-repo config cached in dr.resolved.ForgeConfig).

• [logic-error] internal/cli/repos.go:1340 — commitFn in runReposUpgrade has the same glob-expansion bug: m.ResolveConfig(owner, repo) does exact matching and will fail for any repo matched via a glob pattern. The Upgrade function dispatches commitFn with concrete owner/repo names after glob expansion, so every glob-matched repo hits the error path.
  Remediation: Same fix as scaffoldCommitFn — use resolveConfigWithGlobs, or propagate the already-resolved ForgeConfig from the Upgrade goroutine through the closure.

Low

• [behavior-change] internal/cli/repos.go — The lazy factory design means operations on GitLab manifests with missing GITLAB_TOKEN no longer fail eagerly during dry-run or empty-manifest scenarios (confirmed by test changes in TestReposStatus_GitLabNoToken and TestReposAddCmd_GitLabNoToken). This is generally an improvement (dry-run shouldn't require credentials), but repos add now silently skips the installation-state probe when the forge client can't be created, potentially adding repos without detecting existing installations.

• [error-handling] internal/repos/manifest_edit.go:71 — When clients.ConfigFor(entryForge) returns an error in AddToManifest, the code logs via the progress callback and continues. While probing is best-effort, forge client errors specifically indicate configuration or authentication problems (wrong token, unsupported forge) rather than transient failures. These should be distinguished from probe failures so users know the probe was skipped due to misconfiguration.

---

Prior review findings addressed in this revision:

• [edge-case] (low → resolved): runReposRemove now always initializes uninstallClients via newForgeClientFactory in the else branch — the !opts.dryRun guard was removed, eliminating the nil factory risk.
• [authorization-bypass] (medium → resolved): checkAllForgeScopes now iterates over m.DistinctForges() and validates token permissions for every distinct forge.
• [fail-open] (low → resolved): Both scaffoldCommitFn and commitFn now check the boolean return value from manifest.ResolveConfig() and return a clear error when the repo is not found.
• [test-adequacy] (low → resolved): TestValidate_RejectsSameOwnerMixedForge and TestValidate_AllowsDifferentOwnersDifferentForges now cover the mixed-forge-same-owner validation.
• [missing-documentation] (low → resolved): The same-owner-same-forge constraint is now documented in repo-management.md.

Previous run (12)

Review

Findings

Low

• [edge-case] internal/cli/repos.go — In runReposRemove, when opts.uninstall is true and opts.dryRun is true and opts.testClient is nil, uninstallClients is left as nil (the else if !opts.dryRun branch is skipped). This nil value is passed to Uninstall(), which returns early for dry-run before calling clients.ConfigFor(). The code is currently safe, but the nil factory is an implicit contract — if Uninstall's dry-run path ever changes to validate the factory or call ConfigFor, it would panic. Notably, runReposUninstall does NOT have this guard and always initializes the factory, suggesting the guard in runReposRemove was carried forward from the old single-client code.
  Remediation: Consider initializing the factory unconditionally (removing the !opts.dryRun guard) for defensive safety, or adding a nil-check comment documenting the invariant.

---

Prior review findings addressed in this revision:

• [authorization-bypass] (medium → resolved): checkAllForgeScopes now iterates over m.DistinctForges() and validates token permissions for every distinct forge, fixing the gap where only the default forge's scopes were checked.
• [fail-open] (low → resolved): Both scaffoldCommitFn and commitFn now check the boolean return value from manifest.ResolveConfig() and return a clear error when the repo is not found.
• [test-integrity] (low → resolved): The contextAwareClient override IS preserved through the newTestClientFactory wrapper; the prior finding was incorrect.
• [test-adequacy] (low → resolved): TestValidate_RejectsSameOwnerMixedForge and TestValidate_AllowsDifferentOwnersDifferentForges now cover the mixed-forge-same-owner validation.
• [missing-documentation] (low → resolved): The same-owner-same-forge constraint is now documented in repo-management.md.

Previous run

Review

Findings

Medium

• [authorization-bypass] internal/cli/repos.go:380 — checkPerRepoScopes only runs against the manifest's default forge client (manifest.Defaults.Forge). In a mixed-forge manifest where repos use different forges, the scope check is skipped for the non-default forge. For example, if defaults.forge is github and some repos override to gitlab, the GitLab token's permissions are never validated by checkPerRepoScopes before performing install, uninstall, diff, sync, or upgrade operations on those GitLab repos. This was a pre-existing limitation (scope check only ran on the single client), but this PR makes it architecturally explicit and newly reachable in production by enabling mixed-forge manifests.
  Remediation: Run checkPerRepoScopes (or an equivalent forge-specific scope check) against every distinct forge client that the manifest will use. After expanding globs or walking entries, collect the set of distinct forge names and validate each.

Low

• [test-integrity] internal/repos/batch_install_test.go:1221 — TestBatchInstall_ContextCancellation_Phase1 previously passed a contextAwareClient (which overrides GetRepoVariable to check ctx.Err()) to BatchInstall. The PR now extracts client.FakeClient and wraps it in newTestClientFactory, discarding the context-aware override. The test still validates the primary cancellation path via the semaphore select statement, but it no longer covers the race window where a goroutine passes the semaphore before the cancellation is detected.
  Remediation: Create a contextAwareTestClientFactory that preserves the GetRepoVariable override, or restructure the test to verify cancellation at the factory.ConfigFor level.

• [missing-documentation] docs/guides/getting-started/repo-management.md:83 — The PR adds validation that rejects manifests where the same owner has repos with different forges (manifest.go lines 671–684), but this constraint is not documented in the Multi-forge manifests section. The clear validation error message mitigates this, but users would benefit from knowing the constraint before encountering it.
  Remediation: Add a note explaining that all repos under the same owner must use the same forge.

• [fail-open] internal/cli/repos.go:425 — In the scaffoldCommitFn closure (install) and commitFn closure (upgrade), the second return value from manifest.ResolveConfig(owner, repo) is discarded (rc, _ := ...). For glob-matched repos not listed explicitly in the manifest, the returned ResolvedConfig has an empty Forge field, which falls through to GitHub via ForgeConfigFor's default case. If the repo was actually a GitLab repo matched via glob, the commit function would use the wrong client. The same-owner-same-forge validation mitigates this in practice.
  Remediation: Pass the ResolvedConfig (including resolved forge) from the batch caller rather than re-resolving inside the commit closure, or check the ok return value.

• [test-adequacy] internal/repos/manifest.go:430 — The new mixed-forge-same-owner validation rule (Validate() rejects manifests where the same owner has entries with different forges) has no dedicated unit test. While existing tests exercise Validate(), none specifically cover this new constraint — a regression would go undetected.
  Remediation: Add a test in manifest_test.go that creates a manifest with two entries for the same owner but different forges and asserts that Validate returns an error containing "all repos under the same owner must use the same forge".

---

Labels: PR modifies repos management code (install, status, sync, upgrade, init, uninstall) and forge client abstraction layer

Previous run (13)

Review

Findings

Low

• [edge-case] internal/cli/repos.go — In runReposRemove, when opts.uninstall is true and opts.dryRun is true and opts.testClient is nil, uninstallClients is left as nil (the else if !opts.dryRun branch is skipped). This nil value is passed to Uninstall(), which returns early for dry-run before calling clients.ConfigFor(). The code is currently safe, but the nil factory is an implicit contract — if Uninstall's dry-run path ever changes to validate the factory or call ConfigFor, it would panic. Notably, runReposUninstall does NOT have this guard and always initializes the factory, suggesting the guard in runReposRemove was carried forward from the old single-client code.
  Remediation: Consider initializing the factory unconditionally (removing the !opts.dryRun guard) for defensive safety, or adding a nil-check comment documenting the invariant.

---

Prior review findings addressed in this revision:

• [authorization-bypass] (medium → resolved): checkAllForgeScopes now iterates over m.DistinctForges() and validates token permissions for every distinct forge, fixing the gap where only the default forge's scopes were checked.
• [fail-open] (low → resolved): Both scaffoldCommitFn and commitFn now check the boolean return value from manifest.ResolveConfig() and return a clear error when the repo is not found.
• [test-integrity] (low → resolved): The contextAwareClient override IS preserved through the newTestClientFactory wrapper; the prior finding was incorrect.
• [test-adequacy] (low → resolved): TestValidate_RejectsSameOwnerMixedForge and TestValidate_AllowsDifferentOwnersDifferentForges now cover the mixed-forge-same-owner validation.
• [missing-documentation] (low → resolved): The same-owner-same-forge constraint is now documented in repo-management.md.

Previous run (14)

Review

Findings

Medium

• [authorization-bypass] internal/cli/repos.go:380 — checkPerRepoScopes only runs against the manifest's default forge client (manifest.Defaults.Forge). In a mixed-forge manifest where repos use different forges, the scope check is skipped for the non-default forge. For example, if defaults.forge is github and some repos override to gitlab, the GitLab token's permissions are never validated by checkPerRepoScopes before performing install, uninstall, diff, sync, or upgrade operations on those GitLab repos. This was a pre-existing limitation (scope check only ran on the single client), but this PR makes it architecturally explicit and newly reachable in production by enabling mixed-forge manifests.
  Remediation: Run checkPerRepoScopes (or an equivalent forge-specific scope check) against every distinct forge client that the manifest will use. After expanding globs or walking entries, collect the set of distinct forge names and validate each.

Low

• [test-integrity] internal/repos/batch_install_test.go:1221 — TestBatchInstall_ContextCancellation_Phase1 previously passed a contextAwareClient (which overrides GetRepoVariable to check ctx.Err()) to BatchInstall. The PR now extracts client.FakeClient and wraps it in newTestClientFactory, discarding the context-aware override. The test still validates the primary cancellation path via the semaphore select statement, but it no longer covers the race window where a goroutine passes the semaphore before the cancellation is detected.
  Remediation: Create a contextAwareTestClientFactory that preserves the GetRepoVariable override, or restructure the test to verify cancellation at the factory.ConfigFor level.

• [missing-documentation] docs/guides/getting-started/repo-management.md:83 — The PR adds validation that rejects manifests where the same owner has repos with different forges (manifest.go lines 671–684), but this constraint is not documented in the Multi-forge manifests section. The clear validation error message mitigates this, but users would benefit from knowing the constraint before encountering it.
  Remediation: Add a note explaining that all repos under the same owner must use the same forge.

• [fail-open] internal/cli/repos.go:425 — In the scaffoldCommitFn closure (install) and commitFn closure (upgrade), the second return value from manifest.ResolveConfig(owner, repo) is discarded (rc, _ := ...). For glob-matched repos not listed explicitly in the manifest, the returned ResolvedConfig has an empty Forge field, which falls through to GitHub via ForgeConfigFor's default case. If the repo was actually a GitLab repo matched via glob, the commit function would use the wrong client. The same-owner-same-forge validation mitigates this in practice.
  Remediation: Pass the ResolvedConfig (including resolved forge) from the batch caller rather than re-resolving inside the commit closure, or check the ok return value.

• [test-adequacy] internal/repos/manifest.go:430 — The new mixed-forge-same-owner validation rule (Validate() rejects manifests where the same owner has entries with different forges) has no dedicated unit test. While existing tests exercise Validate(), none specifically cover this new constraint — a regression would go undetected.
  Remediation: Add a test in manifest_test.go that creates a manifest with two entries for the same owner but different forges and asserts that Validate returns an error containing "all repos under the same owner must use the same forge".

---

Labels: PR modifies repos management code (install, status, sync, upgrade, init, uninstall) and forge client abstraction layer

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:20 AM UTC · Completed 2:36 AM UTC
Commit: 2f4f854 · View workflow run →

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:40 AM UTC · Completed 2:55 AM UTC
Commit: 9f5ffa2 · View workflow run →

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:01 AM UTC · Completed 3:17 AM UTC
Commit: c9c184a · View workflow run →

Superseded by updated review

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:52 AM UTC · Completed 4:09 AM UTC
Commit: 17d1ef8 · View workflow run →

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:14 AM UTC · Completed 4:34 AM UTC
Commit: fd24afb · View workflow run →

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:33 AM UTC · Completed 11:47 AM UTC
Commit: b656786 · View workflow run →

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:56 AM UTC · Completed 12:09 PM UTC
Commit: b656786 · View workflow run →

[fullsend-ai-review]

Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:

• internal/cli/repos.go (file-level): Line 1617 · [low] fail-open

checkAllForgeScopes skips scope checking for all non-GitHub forges (forgeName != repos.ForgeGitHub). For GitLab-only manifests, no token scope validation occurs before destructive operations. Intentional because GitLab does not support scope introspection, but misscoped GitLab tokens fail later with unclear per-repo errors. See also: [logic-error] finding at this location.

• internal/cli/repos_test.go:2044: [low] behavior-change

TestReposDiffCmd_GitLabNoToken_WithRepos asserts require.NoError when a manifest has forge: gitlab, actual repos, and no GITLAB_TOKEN. The diff command exits 0 because per-repo forge client errors are surfaced as warnings rather than fatal errors. While this is the intended design, CI pipelines consuming the exit code would see success even though all repos failed to be examined.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:43 PM UTC · Completed 12:56 PM UTC
Commit: db813e5 · View workflow run →

[fullsend-ai-retro]

Retro: PR #5623 — Mixed-forge manifest support

Overall assessment: The workflow performed well. The code agent delivered all 12 design steps from issue #5620 in a single 25-file commit (735 additions, 396 deletions) within 28 minutes. The review agent caught 3 genuine medium-severity bugs across 9 review cycles, and the human domain expert (ggallen) drove all fixes via 7 force-pushes over ~11 hours. The PR merged successfully.

Timeline

Time (UTC)	Event
01:10	Issue created with detailed 12-step design
01:11–01:15	Triage agent ran (success) — run 30229180053
01:20–01:48	Code agent implemented all 12 steps — run 30229547388
02:04	Review Cycle 1: Found medium authorization-bypass in scope checking
02:36	Cycle 2: APPROVED after human fixed scope checking
02:55	Cycle 3: DISMISSED — caught 2 new medium glob-resolution bugs introduced by the human's fix
03:43	Cycle 5: APPROVED after human fixed all medium issues
04:09–04:34	Cycles 6–7: Doc signature cleanup (all low)
11:47	Cycle 8: Clean confirmatory pass after overnight gap
11:54	Human approval (ggallen)
12:09	Cycle 9: Re-review on same commit b656786 (triggered by human approval event)
12:41	Merged

Review agent performance

Genuine bugs caught (3): (1) checkPerRepoScopes only validated the default forge's scopes, missing non-default forges entirely — a medium authorization-bypass. (2–3) scaffoldCommitFn and commitFn used exact-match ResolveConfig instead of ResolveConfigWithGlobs, failing for glob-expanded repos. These were regressions introduced by the human's manual fix, not by the code agent.

False positive (1): Cycle 1 flagged contextAwareClient as lost in newTestClientFactory wrapper. Cycle 2 explicitly acknowledged this was incorrect.

Accepted design limitations (recurring): 4–5 low findings (e.g., checkAllForgeScopes pre-filter timing, GitLab scope-check gap, lazy credential validation) were reported in every cycle from Cycle 5 onward. These represent inherent design tradeoffs that the human accepted but could not suppress.

Evidence for existing issues

• #2959 / #1013 (review finding dedup): This PR provides strong evidence — 4–5 identical low findings were reported across 5 consecutive review cycles (Cycles 5–9). The author never responded to them because they were accepted design limitations, but the review agent had no mechanism to recognize implicit acknowledgment. Implementing dedup would have made cycles 6–9 cleaner.

• #5265 (author response context suppression): Related to the above. The author's pattern — making other changes while leaving specific findings unaddressed across force-pushes — is a form of implicit acknowledgment that the review agent should learn to recognize.

• #4681 (same-commit dedup): Commit b656786 was reviewed twice — Cycle 8 at 11:47 UTC and Cycle 9 at 12:09 UTC. Cycle 9 was triggered by the human's pull_request_review approval event and found no new information.

• #3025 (diminishing-value findings after approval): Cycle 9 ran after both agent approval (11:47) and human approval (11:54), producing only 2 previously-reported low findings. Zero value added.

• #2008 (code agent pre-submit validation): The code agent's one medium gap — scope checking only for the default forge — was a semantic requirement implied by the feature but not explicitly listed as a design step or triage test case. Pre-submit self-validation against acceptance criteria might have caught this.

Autonomy readiness observation

The review agent demonstrated strong value on this PR: it caught 3 genuine medium bugs (including 2 regressions introduced by the human's own fixes), with only 1 false positive. The human's final approval at 11:54 UTC added no findings beyond what the agent had already surfaced. For this class of change (Go interface refactoring with forge-specific code paths), the review agent performed at or above human review level. This is a single data point and the human was deeply involved as the domain expert, so this should not be over-indexed.

Agents repo

Agent definitions resolved from fullsend-ai/agents at digest 78540ca6b412d6f205a5445970d70c60a8e2e5ec.