Skip to content

docs(adr): multi-repo management for per-repo installations#2392

Open
ggallen wants to merge 1 commit into
fullsend-ai:mainfrom
ggallen:worktree-adr-fleet-management
Open

docs(adr): multi-repo management for per-repo installations#2392
ggallen wants to merge 1 commit into
fullsend-ai:mainfrom
ggallen:worktree-adr-fleet-management

Conversation

@ggallen

@ggallen ggallen commented Jun 17, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds ADR XXXX proposing a fullsend repos subcommand group for managing per-repo installations at scale — bulk install, status/drift detection, sync, upgrade, and removal across multiple orgs via a declarative repos.yaml manifest.
  • Adds fullsend repos init for bootstrapping the manifest from existing installations (per-repo and per-org) or greenfield onboarding with interactive repo selection.
  • Includes two separate implementation plans: one for the core repos subcommands (8 PRs across 2 phases), one for repos init (independent, parallelizable).
  • Manifest supports local paths and URLs (following ADR 0038 resource reference model).
  • Cons and risks annotated with whether each is new or inherited from the current per-org/per-repo implementation.

Test plan

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown

Site preview

Preview: https://958ec23a-site.fullsend-ai.workers.dev

Commit: 2da14ddbd29a989bdb10ef9043ca9a58987d672b

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:43 PM UTC · Completed 4:57 PM UTC
Commit: 061706f · View workflow run →

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [broken-forward-reference] docs/ADRs/0051-repos-management.md — ADR 0044 (deprecate per-org installation mode) is referenced 9 times throughout the document without a markdown link. The reference is consistently qualified as "(pending)" with PR docs(adr): deprecate per-org installation mode #2340. No ADR 0044 file exists in the repository. This is an intentional forward reference to an unmerged PR, not a stale reference.

  • [edge-case] docs/ADRs/0051-repos-management.md:307 — Glob expansion via ListOrgRepos excludes private repos. The ADR correctly documents this limitation and notes private repos must be listed explicitly. The plan (PR 3) specifies the fix (either ListOrgReposIncludePrivate method or includePrivate parameter on ListOrgRepos) but defers the choice to implementation time.

Previous run

Review

Findings

High

  • [scope-creep] docs/architecture.md:95 — Removes the ADR 0049 (agent configuration env var convention) bullet from the Agent Harness "Decided" section. ADR 0049 is Accepted and exists on main (docs/ADRs/0049-agent-configuration-env-var-convention.md). This removal erases the documented convention for agent behavioral configuration and is unrelated to repos management (ADR 0051). Removing an accepted ADR reference from architecture.md — the living document that reflects current architectural truth — creates an inconsistency with the accepted ADR.
    Remediation: Revert the removal of the ADR 0049 bullet. If there is a reason to remove it, that should be proposed in a separate PR with explicit justification.

  • [scope-creep] docs/architecture.md:196 — Removes the ADR 0050 (distributed tracing) bullet from the Observability "Decided" section and un-strikes the open question "How do we balance detailed tracing (useful for debugging) with the volume of data agents will produce?", reversing a previous architectural resolution. ADR 0050 is Accepted and exists on main (docs/ADRs/0050-distributed-tracing-instrumentation.md). Other files still reference ADR 0050 as normative (e.g., docs/guides/infrastructure/distributed-tracing.md). These changes are unrelated to repos management.
    Remediation: Revert the removal of the ADR 0050 bullet and the open question reversal. If ADR 0050's resolution is being reconsidered, handle via a superseding ADR, not by silently un-deciding it in an unrelated PR.

Medium

  • [scope-creep] docs/architecture.md:283 — Changes "unanimous rework → triggers fix agent" to "unanimous rework → ready-to-code". This alters the documented coordinator merge algorithm behavior and is unrelated to repos management. The fix agent exists at docs/agents/fix.md on main.
    Remediation: Revert this change or move it to a separate PR focused on workflow coordination changes.

  • [internal-consistency] docs/plans/repos-management.md:43 — PR 8 dependency text says "depends on PR 1 (reuses install types) but is otherwise independent" but the dependency graph diagram directly above shows PR 8 depends on both PR 1 AND PR 3. The per-PR section confirms: "Depends on: PR 1 (reuses types), PR 3 (DeleteRepoVariable, DeleteRepoSecret)." The textual summary contradicts both the diagram and the detailed PR 8 description.
    Remediation: Change the PR 8 textual summary to include the PR 3 dependency.

Low

  • [documentation-accuracy] docs/ADRs/0051-repos-management.md:324 — The ADR states ROLE_APP_IDS maps owner/role to app IDs. In the codebase, ROLE_APP_IDS uses role-only keys (e.g., "coder": "12345"). The mintcore.RoleOnlyAppIDs() function explicitly filters out owner/role keys as legacy. The phrasing is imprecise and could mislead implementers building the multi-org support described in this section.

  • [edge-case] docs/plans/repos-management.md:538 — The ADR documents that Phase 2 re-checks the guard variable before provisioning to narrow the TOCTOU window, but the plan's Phase 2 description goes straight to EnsureOrgInMint and ProvisionWIF without mentioning a guard variable re-check.

  • [edge-case] docs/ADRs/0051-repos-management.md:307 — The ADR correctly documents that glob patterns exclude private repos and notes private repos must be listed explicitly. The plan (PR 3) defers the approach (extend ListOrgRepos or add includePrivate parameter) without specifying which.


Labels: PR adds ADR and implementation plans for per-repo installation management tooling.

Previous run (2)

Review

Findings

Medium

  • [internal-consistency] docs/plans/repos-management.md:43 — PR 8 dependency text says "depends on PR 1 (reuses install types) but is otherwise independent" but the dependency graph diagram directly above shows PR 8 depends on both PR 1 AND PR 3. The per-PR section at line 859 also confirms: "Depends on: PR 1 (reuses types), PR 3 (DeleteRepoVariable, DeleteRepoSecret)." The textual summary contradicts both the diagram and the detailed PR 8 description.
    Remediation: Change line 43 text to include the PR 3 dependency.

Low

  • [documentation-accuracy] docs/ADRs/0049-repos-management.md:278 — The ADR states ROLE_APP_IDS maps owner/role to app IDs. In the codebase, ROLE_APP_IDS uses role-only keys (e.g., "triage": "12345"), with legacy org/role keys preserved only during migration. The phrasing is imprecise and could mislead implementers building multi-org support.

  • [logic-error] docs/plans/repos-management.md:915 — The repos remove branch-protection fallback describes using CommitFilesToBranch + PR creation to delete the workflow file. However, the current CommitFilesToBranch forge interface takes []TreeFile which creates/updates files — it has no mechanism for expressing file deletions. A minor interface extension (e.g., a deletion flag on TreeFile) will be needed at implementation time.

  • [ADR-structure-consistency] docs/ADRs/0049-repos-management.md — ADR uses a non-standard "Pros and cons" section in addition to "Consequences." Existing ADRs use either Options or Consequences, not both. The Pros/cons content adds value with existing/new annotations but is a style deviation.

  • [Decision-section-structure] docs/ADRs/0049-repos-management.md:137 — Decision section uses deep nesting (h2 > h3 > h4) with 8 level-4 headings under "Subcommand details." This is atypical for ADRs in this repo, though reasonable given the number of commands being defined.

  • [edge-case] docs/ADRs/0049-repos-management.md:265ListOrgRepos excludes private repos. The ADR correctly identifies this limitation and the plan addresses it, but the solution approach (extend ListOrgRepos or add includePrivate parameter) is deferred without specifying which.

  • [non-existent-reference] docs/ADRs/0049-repos-management.md:939 — ADR 0044 is referenced throughout without a hyperlink, while all other ADR references include links. Intentional given pending status (PR docs(adr): deprecate per-org installation mode #2340).

  • [missing-cross-reference] docs/reference/installation.md:24 — The installation guide describes bulk enrollment as a per-org advantage but does not mention ADR 0049's repos management tooling for per-repo mode. Forward-looking enhancement since the tooling is not yet implemented.

  • [missing-cross-reference] docs/ADRs/0033-per-repo-installation-mode.md — ADR 0033 does not include a forward-reference annotation to ADR 0049, which addresses the operational gaps (no bulk operations, no enrollment inventory, no drift detection) identified in ADR 0049's context section.

  • [ADR-cross-reference-format] docs/ADRs/0049-repos-management.md:709 — ADR 0044 reference format is inconsistent — the "(pending)" qualifier is repeated 8 times throughout the document. A single explanatory note in the Context section would be less verbose.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces declarative manifest-driven orchestration as a new organizing principle in CLI design. Architecturally valid and well-motivated — reuses the existing forge.Client abstraction and avoids creating new infrastructure.

  • [dependency-chain] docs/ADRs/0049-repos-management.md:30 — ADR 0049 depends on ADR 0048 (automatic updates with --upstream-ref). Core commands work independently; only version-management commands (repos upgrade, repos upgrade-mint) require ADR 0048.

Previous run (3)

Review

Findings

Medium

  • [logic-error] docs/plans/repos-management.md:904 — In BatchInstall Phase 2, EnsureOrgInMint is called once per unique org before the per-repo WIF provisioning loop. The plan does not specify what happens if EnsureOrgInMint fails for an org — all repos in that org would be affected, but the plan does not describe whether those repos should be moved to Failed or whether the batch should abort. By contrast, per-repo ProvisionWIF/RegisterPerRepoWIF failure handling is well-specified (lines 1917–1919).
    Remediation: Specify that if EnsureOrgInMint fails for an org, all repos in toInstall belonging to that org are moved to BatchInstallResult.Failed with the error, and excluded from Phase 2 per-repo WIF provisioning and Phase 3.

Low

  • [internal-consistency] docs/ADRs/0049-repos-management.md:258 — The field resolution pseudocode uses || (logical-or) notation, which implies that null falls through to the next candidate. Two paragraphs later, the prose states that explicit YAML null stops the fallback chain. The resolveField implementation in the plan (repos-management.md:1617) correctly implements the prose, but the pseudocode will mislead implementers who read only the ADR.

  • [documentation-accuracy] docs/plans/repos-init.md:149 — The plan says per-org enrolled repos should use config.DefaultUpstreamRef (internal/config/config.go:15) as their discovered fullsend_ref. This value is v0, a major-version floating tag used in reusable workflow uses: lines — not a concrete release version like v2.3.0. Storing v0 as the manifest's fullsend_ref conflates the workflow-call ref with the ADR 0048 --upstream-ref version concept. Downstream, repos upgrade would always "upgrade" these repos.
    Remediation: Extract the actual ref from the workflow file's uses: line (same as per-repo discovery), or omit the ref and let it inherit from defaults.fullsend_ref.

  • [edge-case] docs/ADRs/0049-repos-management.md:275 — Glob expansion via ListOrgRepos excludes private repos. The ADR acknowledges this limitation (lines 281–287) and the implementation plan (PR 3) mentions extending ListOrgRepos, but the extension is not scoped to a specific PR milestone. If it ships after repos install, operators using glob patterns will silently miss private repos.

  • [missing-cross-reference] docs/reference/installation.md:24 — The installation guide describes 'enrolling many repositories' as a reason to choose per-org mode but does not mention the new repos management tooling (ADR 0049) which provides bulk management for per-repo mode. This is a forward-looking documentation enhancement since the repos tool is not yet implemented.

  • [missing-cross-reference] docs/ADRs/0033-per-repo-installation-mode.md — ADR 0033 does not include a forward-reference annotation to ADR 0049, which addresses the operational gaps (no bulk operations, no enrollment inventory, no drift detection) identified in ADR 0049's context section. Minor annotations are permitted per the ADR template.

  • [non-existent-reference] docs/ADRs/0049-repos-management.md:955 — ADR 0044 is referenced throughout without a markdown link, consistently marked as "(pending)" with PR docs(adr): deprecate per-org installation mode #2340. No ADR 0044 file exists in the repository. This is a forward reference to an unmerged PR, not a stale reference.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces declarative manifest-driven orchestration as a new organizing principle in CLI design. Architecturally valid and well-motivated — reuses the existing forge.Client abstraction and avoids creating new infrastructure.

  • [dependency-chain] docs/ADRs/0049-repos-management.md:50 — ADR 0049 depends on ADR 0048 (automatic updates with --upstream-ref) which is noted as "assumed implemented." Core commands work independently; only version-management commands (repos upgrade, repos upgrade-mint) require ADR 0048.

Previous run (4)

Review

Findings

Medium

  • [logic-error] docs/plans/repos-management.md:918 — In BatchInstall Phase 3, the plan iterates over all repos in toInstall and looks up wifProviders[repo] for the WIF provider provisioned in Phase 2. However, Phase 2 errors are not described as filtering repos out of the Phase 3 input. If ProvisionWIF or RegisterPerRepoWIF fails for a repo in Phase 2, that repo will have no entry in wifProviders, and Phase 3 will call Install() with an empty WIFProvider string — writing an empty secret and leaving the repo in a broken state. By contrast, repos remove (lines 2234–2244) explicitly gates Phase 2 on Phase 1 success, demonstrating the pattern is known but omitted here.
    Remediation: Add explicit gating — after Phase 2, partition toInstall into wifSucceeded and wifFailed based on whether wifProviders[repo] was populated. Run Phase 3 only for wifSucceeded repos. Move wifFailed repos to BatchInstallResult.Failed.

Low

  • [documentation-accuracy] docs/plans/repos-management.md:893 — The repos remove plan references RemoveRepoFromMint at "line 1384" of provisioner.go, but the method is actually at line 1493. Line 1384 is inside ProvisionWIF, a different method.
    Remediation: Update the line reference to 1493, or remove the line number and reference the method by name only.

  • [edge-case] docs/ADRs/0049-repos-management.md:275 — Glob expansion via ListOrgRepos excludes private repos. The ADR acknowledges this limitation (lines 281–287) and states the implementation must extend ListOrgRepos, but the implementation plan does not scope this extension into any specific PR. Until addressed, private repos must be listed explicitly.

  • [non-existent-reference] docs/ADRs/0049-repos-management.md:955 — ADR 0044 is referenced throughout without a markdown link, consistently marked as "(pending)" with PR docs(adr): deprecate per-org installation mode #2340. No ADR 0044 file exists in the repository. This is a forward reference to an unmerged PR, not a stale reference.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces declarative manifest-driven orchestration as a new organizing principle in CLI design. Architecturally valid and well-motivated — reuses the existing forge.Client abstraction and avoids creating new infrastructure.

  • [dependency-chain] docs/ADRs/0049-repos-management.md:50 — ADR 0049 depends on ADR 0048 (automatic updates with --upstream-ref) which is noted as "assumed implemented." Core commands work independently; only version-management commands (repos upgrade, repos upgrade-mint) require ADR 0048.

  • [edge-case] docs/plans/repos-management.md:591 — Upgrade semver logic correctly distinguishes non-semver targets (step 3: skip floating tags like latest) from non-semver current refs (step 5: proceed with upgrade since comparison is not possible). Internally consistent.

Previous run (5)

Review

Findings

Medium

  • [logic-error] docs/plans/repos-management.md:893 — In the repos remove plan (PR 8), Phase 1 correctly gates variable/secret deletion on workflow file deletion success. However, Phase 2 (WIF cleanup) runs unconditionally for all repos — there is no check that Phase 1 succeeded before deregistering a repo from the mint's PER_REPO_WIF_REPOS and deleting its WIF provider. If Phase 1 fails (e.g., workflow deletion blocked by branch protection with no PR fallback), Phase 2 would still delete the WIF provider, leaving the repo with a workflow that references a non-existent WIF provider.
    Remediation: In Phase 2, skip WIF cleanup for repos whose Phase 1 failed. The RemoveResult.Success field from Phase 1 can gate Phase 2 execution per repo.

Low

  • [edge-case] docs/ADRs/0049-repos-management.md:275 — Glob expansion via ListOrgRepos excludes private repos. The ADR acknowledges this limitation (lines 281-287) and states the implementation must extend ListOrgRepos, but the implementation plan does not scope this extension into any specific PR (PR 3 adds ListRepoVariables, DeleteRepoVariable, DeleteRepoSecret but not a private-repo-aware ListOrgRepos variant). Until addressed, private repos must be listed explicitly.

  • [logic-error] docs/plans/repos-management.md:308NullableString.MarshalYAML returns (nil, nil) for both the Null case and the !Set case. While IsZero() correctly returns false for explicit nulls (preventing omitempty from stripping them), MarshalYAML still produces nil output, which yaml.v3 serializes as an omitted field. Explicit-null semantics are lost on round-trip (LoadManifest → modify → Marshal).
    Remediation: Return a *yaml.Node with tag !!null when n.Null is true.

  • [edge-case] docs/plans/repos-management.md:793 — The repos remove plan includes a branch protection fallback (CommitFilesToBranch + PR creation) for workflow file deletion, addressing the prior finding. However, the plan does not specify how to distinguish branch-protection errors (HTTP 403/422) from other failures (insufficient permissions, network errors). Falling back on all errors could mask real failures.

  • [non-existent-reference] docs/ADRs/0049-repos-management.md:955 — ADR 0044 (deprecate per-org installation mode, PR docs(adr): deprecate per-org installation mode #2340) is referenced throughout the document and in the References section without a markdown link. The ADR consistently marks it as "(pending)" and explicitly states repos management can be built independently — this is a forward reference to an unmerged PR, not a stale reference.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces declarative manifest-driven orchestration as a new organizing principle in CLI design. Architecturally valid and well-motivated — reuses the existing forge.Client abstraction and avoids creating new infrastructure.

  • [dependency-chain] docs/ADRs/0049-repos-management.md — ADR 0049 depends on ADR 0048 (automatic updates with --upstream-ref) which is noted as "assumed implemented." Core commands work independently; only version-management commands (repos upgrade, repos upgrade-mint) require ADR 0048.

  • [edge-case] docs/ADRs/0049-repos-management.md:640repos remove does not remove repos from the manifest — the operator edits repos.yaml manually. This is a deliberate design choice documented at line 660.

  • [edge-case] docs/plans/repos-management.md:591 — Upgrade semver logic correctly distinguishes non-semver targets (step 3: skip floating tags like latest) from non-semver current refs (step 5: proceed with upgrade since comparison is not possible). Internally consistent.

  • [cross-reference-format] docs/ADRs/0049-repos-management.md:955 — ADR 0044 listed without markdown link in References section. Correct for a pending ADR with no file to link to. See also: [non-existent-reference] finding at this location.

  • [future-api-reference] docs/ADRs/0049-repos-management.md — Section 6 uses present tense ("The repos tool adds ListRepoVariables") for a method not yet in the forge interface. Standard ADR convention — describes the decision, not current state. The implementation plan correctly scopes this to PR 3.

Previous run (6)

Review

Findings

Medium

  • [logic-error] docs/plans/repos-management.md:893repos remove Phase 1 deletes variables and secrets in parallel with deleting the workflow file ("Steps 1–3 are independent per repo and run concurrently"). If workflow file deletion fails (e.g., branch protection on the default branch), variables and secrets are still deleted, leaving the repo in a broken state: the workflow exists but FULLSEND_MINT_URL, FULLSEND_GCP_REGION, and FULLSEND_GCP_PROJECT_ID values it depends on are gone. Subsequent workflow runs will fail. See also: [edge-case] finding below about missing branch protection fallback.
    Remediation: Restructure Phase 1 to delete the workflow file first; only proceed to delete variables and secrets if that succeeds. Alternatively, add a branch-protection fallback (as repos upgrade already describes) and gate variable/secret cleanup on successful workflow removal.

  • [edge-case] docs/ADRs/0049-repos-management.md:275 — Glob expansion calls ListOrgRepos which excludes private repos (github.go:337 filters r.Private). The implementation plan (repos-management.md line ~1582) correctly identifies this gap and says ListOrgRepos must be extended, but the ADR itself does not record this constraint. Operators using globs like acme-corp/* on orgs with private repos would see them silently excluded.
    Remediation: Add a note in the ADR's glob expansion section that ListOrgRepos must include private repos for per-repo mode, or document that glob patterns only match public repos and private repos must be listed explicitly.

Low

  • [internal-consistency] docs/plans/repos-management.md:119 — The proposed WIFProvisioner interface defines DiscoverMint as returning (*MintInfo, error), but the actual Provisioner.DiscoverMint method (provisioner.go:329) returns (*MintDiscovery, error). The plan does not define a MintInfo type anywhere — the implementer will need to reconcile the type name.

  • [edge-case] docs/plans/repos-management.md:793repos upgrade describes a branch protection fallback for scaffold commits (CommitFiles or CommitFilesToBranch + PR), but repos remove uses DeleteFile with no branch protection fallback. On repos with branch protection, DeleteFile will fail, blocking the removal flow. See also: [logic-error] finding above about Phase 1 ordering.

  • [logic-error] docs/plans/repos-management.md:308 — The resolveField function takes (override, fallback NullableString, builtinDefault string) but DefaultsConfig uses plain string fields, not NullableString. The plan does not show how DefaultsConfig string values are converted to NullableString for the fallback parameter.

Info

  • [edge-case] docs/ADRs/0049-repos-management.md:640repos remove does not remove repos from the manifest — the operator edits repos.yaml manually. If forgotten, the next repos install will re-provision the removed repo.

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [cross-reference-format] docs/ADRs/0049-repos-management.md:947 — ADR 0049 references "ADR 0044" without a markdown link in the References section. Other ADR cross-references use linked format. The omission is intentional — ADR 0044 is pending and no file exists to link to.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [dependency-chain] docs/ADRs/0049-repos-management.md — ADR 0049 depends on ADR 0048 (automatic updates with --upstream-ref) which is noted as "assumed implemented." Core commands work independently; only version-management commands (repos upgrade, repos upgrade-mint) require ADR 0048.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces declarative manifest-driven orchestration as a new organizing principle in CLI design. Architecturally valid and well-motivated — reuses the existing forge.Client abstraction and avoids creating new infrastructure.

Previous run (7)

Review

Findings

Medium

  • [edge-case] docs/plans/repos-management.md:280 — Glob expansion uses ListOrgRepos which, per its interface contract in internal/forge/forge.go:162-177, excludes archived repos, forks, and private repos. The exclusion of private repos was designed for per-org mode where agent workflows run on a public .fullsend config repo. In per-repo mode (which this ADR exclusively targets), agents run on the target repo itself, so private repos should be eligible. Glob patterns like acme-corp/* would silently exclude all private repos without warning. The ListOrgRepos interface comment itself acknowledges: "Private repo support requires per-repo .fullsend mode where agents run on the target repo."
    Remediation: Either document this limitation in the ADR's glob expansion section, or note that ListOrgRepos needs a variant/parameter that includes private repos for per-repo mode.

Low

  • [internal-consistency] docs/ADRs/0049-repos-management.md:562 — The repos upgrade example output shows partner-org/shared-sdk v2.1.0 (pinned in manifest, skipped) and acme-corp/legacy-service v2.1.0 (pinned in manifest, skipped). Both repos are pinned to v2.1.0 and are already at v2.1.0. Per the implementation plan's upgrade logic (step 2: target = per-repo fullsend_ref = v2.1.0; step 4: skip if current == target), the skip reason would be "already current", not "pinned in manifest, skipped". The example implies pinned repos are always skipped regardless of version, contradicting the plan's logic.
    Remediation: Align the example output messages with the plan's skip logic, or add a step in the plan that uses a "pinned" skip reason when the target ref comes from a per-repo override.

  • [logic-error] docs/plans/repos-management.md:570BatchInstall Phase 2 calls ProvisionWIF(ctx) which returns (string, error), but the pseudocode does not explicitly show the returned provider name being stored and threaded into Phase 3's InstallConfig.WIFProvider field. The plan states "WIFProvider set to the provider name from Phase 2" but the data flow (storing the return value per-repo for later use) is implicit.
    Remediation: Add a comment or pseudocode line showing the provider name is stored per-repo for use in Phase 3.

  • [edge-case] docs/ADRs/0049-repos-management.md:246 — The ADR states "Empty-string and zero-value overrides are treated as unset and fall through to defaults." The implementation plan's NullableString type distinguishes three states: omitted (Set=false), explicit null (Set=true, Null=true), and explicit value (Set=true, Null=false). An explicit empty string would be Set=true, Null=false, Value="" — a "set" value under NullableString semantics that would NOT fall through to defaults, contradicting the ADR prose.
    Remediation: Align the ADR prose with the NullableString implementation, or add logic in ResolveConfig to treat Set=true, Value="" as falling through.

Info

  • [edge-case] docs/plans/repos-management.md:859repos remove Phase 1 deletes the workflow file via DeleteFile but does not mention a fallback for branch-protected repos. The upgrade command (PR 7) accounts for this with CommitFilesToBranch + PR creation, but remove does not.

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces a new organizing principle in CLI design (operations on a collection of installations vs. existing per-layer commands). This is architecturally valid and well-motivated.

  • [cross-reference-format] docs/ADRs/0049-repos-management.md:927 — ADR 0049 references "ADR 0044" without a markdown link in the References section, while other ADR cross-references use linked format. This is a pending ADR so no file exists to link to.

Previous run (8)

Review

Findings

Low

  • [api-behavior-claim] docs/plans/repos-management.md:506NullableString defines a custom UnmarshalYAML but the plan does not mention a corresponding MarshalYAML. If implemented as written, yaml.v3 would serialize NullableString as a three-field struct rather than a scalar, breaking round-trip through Marshal()/LoadManifest(). An implementer following this plan would likely discover the issue during development when writing tests for Marshal(), but noting it explicitly in the plan would prevent the gap.
    Remediation: Add a note to the plan specifying that NullableString needs a MarshalYAML method (returning nil for Null=true, n.Value for Set=true) and optionally IsZero() for omitempty support.

  • [logic-error] docs/plans/repos-management.md:327 — The PR dependency graph diagram shows PR 8 (remove) visually dangling with no incoming arrows. The prose text at the PR 8 section states "Depends on: PR 1 (reuses types), PR 3 (DeleteRepoVariable, DeleteRepoSecret)" but these edges are absent from the ASCII diagram. The prose immediately below the diagram does state the correct dependencies, mitigating the risk.
    Remediation: Add edges from PR 1 and PR 3 to PR 8 in the dependency graph diagram.

Info

  • [edge-case] docs/plans/repos-management.md:506 — Without an IsZero() method on NullableString, omitempty behavior relies on reflect deep-equal to the zero value. This works correctly for the current struct but is fragile if fields are added later.

  • [cross-reference-format] docs/ADRs/0049-repos-management.md:717 — ADR 0049 references "ADR 0044" without a markdown link in the References section, while other ADR cross-references use linked format. This is a pending ADR so no file exists to link to.

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces a new organizing principle in CLI design (operations on a collection of installations vs. existing per-layer commands). This is architecturally valid and well-motivated.

Previous run (9)

Review

Findings

High

  • [api-behavior-claim] docs/plans/repos-management.md:1535 — The RepoEntry.UnmarshalYAML method uses the yaml.v2 signature func(unmarshal func(interface{}) error) error, but the project uses gopkg.in/yaml.v3 (go.mod line 17). In yaml.v3, the Unmarshaler interface requires UnmarshalYAML(value *yaml.Node) error. The v2-style signature will silently not be called by the yaml.v3 decoder — RepoEntry will use default struct unmarshaling, which means string-form repo entries (- acme-corp/api-server) will fail to parse. This is internally inconsistent with the NullableString.UnmarshalYAML on line 1507, which correctly uses the yaml.v3 *yaml.Node signature.
    Remediation: Change the RepoEntry.UnmarshalYAML signature to func(r *RepoEntry) UnmarshalYAML(node *yaml.Node) error and use node.Decode() instead of calling unmarshal(). Check node.Kind == yaml.ScalarNode to detect string form, then node.Decode(&s) for string or node.Decode((*raw)(r)) for object form.

Low

  • [edge-case] docs/plans/repos-management.md:1507 — The NullableString.UnmarshalYAML correctly handles the !!null tag, but RepoEntry fields use omitempty YAML tags. If Marshal() (from the repos-init plan) is used for round-tripping, omitempty will skip fields where NullableString has Set=true, Null=true (explicit null), losing the explicit-null distinction. The scenario is speculative (no plan describes read-modify-write round-tripping), but worth noting for implementation.

  • [logic-error] docs/plans/repos-management.md:1838 — In PR 5 (batch install), Phase 2 calls EnsureOrgInMint(ctx, mintURL, org) per repo, but EnsureOrgInMint operates at the org level. Calling it once per repo is redundant for repos in the same org — each call performs a read-modify-write on Cloud Run environment variables, adding unnecessary sequential latency. The operation is idempotent, so this is a performance issue rather than a correctness error.
    Remediation: Deduplicate EnsureOrgInMint calls by org — collect unique orgs from toInstall, call once per unique org before iterating repos for RegisterPerRepoWIF.

  • [cross-reference-format] docs/ADRs/0049-repos-management.md:717 — ADR 0049 references "ADR 0044" without a markdown link, while other ADR cross-references use the bracketed [ADR 00XX](00XX-...) format. The inconsistency is intentional — ADR 0044 does not yet exist in the repo (PR docs(adr): deprecate per-org installation mode #2340, pending) — but the unlinked format is less navigable.

  • [edge-case] docs/plans/repos-management.md:2029 — The upgrade logic step 3 says "Skip if target is a non-semver ref" (e.g., latest) and step 5 says "If current is not valid semver, proceed with the upgrade." These steps are consistent but their interaction is subtle: when an operator changes their manifest from latest to a semver target, step 3 does not skip (target is semver) and step 5 proceeds (current is non-semver), which is the correct behavior.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces a new organizing principle in CLI design (operations on a collection of installations vs. existing per-layer commands). This is architecturally valid and well-motivated.

Previous run (10)

Review

Findings

Medium

  • [api-behavior-claim] docs/plans/repos-management.md:1556 — The plan claims Go's yaml.v3 unmarshals YAML null/~ as a non-nil pointer to the zero value when the target is *string, enabling three-state distinction (omitted=nil, null=non-nil-empty, set=non-nil-value). This is incorrect: yaml.v3 unmarshals both omitted fields and explicit null into a nil *string pointer. The two cases are indistinguishable. The custom UnmarshalYAML (line 1513) handles string-vs-object form but does not address the null distinction. Consequently, the null-stops-fallback-chain semantics promised by ADR 0049 (lines 265–268: "A null override stops the fallback chain") cannot be implemented with the proposed *string type alone.
    Remediation: Use a custom wrapper type (e.g., NullableString with its own UnmarshalYAML that sets a present boolean flag on any YAML node including null) or use yaml.Node-based parsing to distinguish between absent keys and explicitly-null values.

Low

  • [broken-forward-reference] docs/ADRs/0049-repos-management.md:717 — ADR 0049 references "ADR 0044 (deprecate per-org installation mode, PR docs(adr): deprecate per-org installation mode #2340, pending)" but no file matching docs/ADRs/0044* exists in the repository. The text consistently qualifies it as "pending", which mitigates confusion, but the forward reference remains unresolvable.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces a new organizing principle in CLI design (operations on a collection of installations vs. existing per-layer commands). This is architecturally valid and well-motivated.

  • [sub-agent-failure] — The style-conventions sub-agent did not return findings: could not access PR branch files.

  • [sub-agent-failure] — The docs-currency sub-agent did not return findings: could not access PR branch files.

Previous run (11)

Review

Findings

Medium

  • [type-system-mismatch] docs/plans/repos-management.md:1486 — The ADR (lines 264–268) promises that YAML null (~ or null) overrides stop the fallback chain, making the resolved value empty rather than inheriting the default. However, the RepoEntry struct uses plain string fields (not *string pointers). Go's yaml.v3 unmarshals both an omitted field and an explicit null to the zero value "", making them indistinguishable. The ResolveConfig function cannot differentiate "field omitted" (should inherit default) from "field explicitly set to null" (should resolve to empty). The same issue applies to DefaultsConfig.
    Remediation: Use *string pointer fields in RepoEntry and DefaultsConfig for overridable fields. A nil pointer means "omitted, inherit default"; a non-nil pointer to an empty string means "explicitly cleared." Alternatively, use a custom type with an IsSet flag.

  • [api-contract-mismatch] docs/plans/repos-management.md:1416 — The proposed WIFProvisioner interface defines EnsureOrgInMint(ctx context.Context, org string, appIDs map[string]int64) error, but the actual Provisioner.EnsureOrgInMint signature is func (p *Provisioner) EnsureOrgInMint(ctx context.Context, expectedURL string, org string) error. The interface drops the expectedURL parameter and adds an appIDs parameter that the real method does not accept. The BatchInstall call site (line 1811) uses the proposed signature, which will not compile against the real method.
    Remediation: Align the WIFProvisioner.EnsureOrgInMint signature with the actual provisioner. Either include expectedURL as a parameter or store it in the provisioner constructor. Remove the appIDs parameter unless the method is being redesigned.

Low

  • [internal-consistency] docs/plans/repos-management.md:1333 — The dependency graph declares "PR 7 depends on PR 6" but the upgrade algorithm in PR 7 does not call Diff() or Status() from PR 6. It reads the workflow file directly via extractWorkflowRef() (first defined in PR 4). The dependency appears to be only on shared helper functions, not on diff/sync logic proper.

  • [broken-forward-reference] docs/ADRs/0049-repos-management.md:714 — ADR 0049 references "ADR 0044 (deprecate per-org installation mode, PR docs(adr): deprecate per-org installation mode #2340)" multiple times but no file matching docs/ADRs/0044* exists in the repository. The reference is consistently qualified as "(pending)" in the current version.

  • [internal-consistency] docs/plans/repos-init.md:1041 — The plan references internal/cli/admin.go:431 for "enrollment prompts" described as an interactive checklist (arrow keys, space to toggle, enter to confirm). The actual promptEnrollment at that location is a simple binary a/n prompt, not a multi-select checklist. The repos init command's proposed interactive selection is a new UX pattern, not an existing one being reused.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [forward-reference-specificity] docs/ADRs/0049-repos-management.md:31 — ADR 0049 states version-management commands "require its --upstream-ref mechanism" when referring to ADR 0048. ADR 0048 itself does not mention --upstream-ref — the detail lives in its implementation plan.

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The fullsend repos command group expands the CLI surface area beyond what ADR 0033 explicitly authorized. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The fullsend repos subcommand group introduces a new organizing principle in CLI design (operations on a collection of installations vs. existing per-layer commands). This is architecturally valid and well-motivated.

  • [documentation-coherence] docs/architecture.md:48 — Repos management placed in "Agent Infrastructure" section. The placement is defensible given other installation/provisioning decisions in the same section.

Previous run (12)

Review

Findings

Low

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The proposed fullsend repos command group expands the CLI surface area. ADR 0033 established per-repo installation but did not explicitly authorize multi-repo tooling. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on, but maintainer confirmation at merge time is appropriate.

  • [architectural-fit] docs/ADRs/0049-repos-management.md:897 — The "Future enhancements" section proposes that repos install subsume github setup, a significant UX shift that would benefit from its own ADR when pursued. The section placement already signals out-of-scope, but an explicit note would strengthen the boundary.

  • [internal-consistency] docs/plans/repos-management.md:1333 — The dependency graph declares "PR 7 depends on PR 6 (upgrade checks for drift before proceeding)" but the upgrade algorithm never explicitly calls Diff() or Status() from PR 6. The stated dependency rationale does not match the described implementation.

  • [internal-consistency] docs/plans/repos-management.md:2090repos remove lists per-repo steps sequentially without noting which can run in parallel and which must be sequential. Steps 1-3 (forge operations) are independent per-repo and could parallelize, while step 4 (WIF deregistration) has the same serialization constraint as install.

  • [broken-forward-reference] docs/ADRs/0049-repos-management.md:714 — ADR 0049 references "ADR 0044 (deprecate per-org installation mode, PR docs(adr): deprecate per-org installation mode #2340)" multiple times but no file matching docs/ADRs/0044* exists in the repository. The reference is to a pending/future ADR and is qualified as "(pending)" in some places but not consistently.

  • [edge-case] docs/ADRs/0049-repos-management.md:265 — The field resolution rule "Empty-string and zero-value overrides are treated as unset and fall through to defaults" makes it impossible to explicitly clear a per-repo field. YAML supports explicit null (~ or null) as a distinct sentinel that could differentiate "omitted" from "explicitly cleared."

  • [internal-consistency] docs/plans/repos-management.md:1529 — The manifest validation rule says "Glob patterns are valid (single * at end of org prefix)" but glob expansion uses filepath.Match which supports richer patterns like org/api-*. If only org/* is valid, filepath.Match is unnecessary; if richer patterns are supported, the validation rule is too restrictive.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The proposed fullsend repos subcommand group introduces a new organizing principle in the CLI design (operations on a collection of installations vs. existing per-layer commands). This is architecturally valid but represents a new dimension worth noting.

  • [forward-reference-specificity] docs/ADRs/0049-repos-management.md:31 — ADR 0049 states version-management commands "require its --upstream-ref mechanism" when referring to ADR 0048. ADR 0048 itself does not mention --upstream-ref — the detail lives in its implementation plan.

  • [documentation-coherence] docs/architecture.md:48 — Repos management placed in "Agent Infrastructure" section. The placement is defensible given other installation/provisioning decisions in the same section.

Previous run (13)

Review

Findings

Low

  • [scope-alignment] docs/ADRs/0049-repos-management.md — The proposed fullsend repos command group expands the CLI surface area. ADR 0033 established per-repo installation but did not explicitly authorize multi-repo tooling. The roadmap mentions per-repo installation and multi-repo capabilities in the current phase, making this a natural follow-on, but maintainer confirmation at merge time is appropriate.

  • [architectural-fit] docs/ADRs/0049-repos-management.md:897 — The "Future enhancements" section proposes that repos install subsume github setup, a significant UX shift that would benefit from its own ADR when pursued. The section placement already signals out-of-scope, but an explicit note would strengthen the boundary.

  • [internal-consistency] docs/plans/repos-management.md:1333 — The dependency graph declares "PR 7 depends on PR 6 (upgrade checks for drift before proceeding)" but the upgrade algorithm never explicitly calls Diff() or Status() from PR 6. The stated dependency rationale does not match the described implementation.

  • [internal-consistency] docs/plans/repos-management.md:2090repos remove lists per-repo steps sequentially without noting which can run in parallel and which must be sequential. Steps 1-3 (forge operations) are independent per-repo and could parallelize, while step 4 (WIF deregistration) has the same serialization constraint as install.

  • [broken-forward-reference] docs/ADRs/0049-repos-management.md:714 — ADR 0049 references "ADR 0044 (deprecate per-org installation mode, PR docs(adr): deprecate per-org installation mode #2340)" multiple times but no file matching docs/ADRs/0044* exists in the repository. The reference is to a pending/future ADR and is qualified as "(pending)" in some places but not consistently.

  • [edge-case] docs/ADRs/0049-repos-management.md:265 — The field resolution rule "Empty-string and zero-value overrides are treated as unset and fall through to defaults" makes it impossible to explicitly clear a per-repo field. YAML supports explicit null (~ or null) as a distinct sentinel that could differentiate "omitted" from "explicitly cleared."

  • [internal-consistency] docs/plans/repos-management.md:1529 — The manifest validation rule says "Glob patterns are valid (single * at end of org prefix)" but glob expansion uses filepath.Match which supports richer patterns like org/api-*. If only org/* is valid, filepath.Match is unnecessary; if richer patterns are supported, the validation rule is too restrictive.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only by an org member, so this is a process note rather than a blocker.

  • [architectural-coherence] docs/ADRs/0049-repos-management.md — The proposed fullsend repos subcommand group introduces a new organizing principle in the CLI design (operations on a collection of installations vs. existing per-layer commands). This is architecturally valid but represents a new dimension worth noting.

  • [forward-reference-specificity] docs/ADRs/0049-repos-management.md:31 — ADR 0049 states version-management commands "require its --upstream-ref mechanism" when referring to ADR 0048. ADR 0048 itself does not mention --upstream-ref — the detail lives in its implementation plan.

  • [documentation-coherence] docs/architecture.md:48 — Repos management placed in "Agent Infrastructure" section. The placement is defensible given other installation/provisioning decisions in the same section.

Previous run (14)

Review

Findings

Medium

  • [edge-case] docs/plans/repos-management.md:1988 — The upgrade algorithm performs semver comparison (step 4: "Skip if current > target") but never handles non-semver ref values. The ADR explicitly supports fullsend_ref: latest as a floating tag, and the upgrade output example shows "floating tag, skipped" — but the implementation plan's algorithm steps don't include a check for non-semver refs before the semver comparison. Comparing v2.3.0 > latest would produce undefined results.
    Remediation: Add a step before the semver comparison: skip if target ref is not a valid semver string (e.g., latest).

Low

  • [internal-consistency] docs/plans/repos-management.md:1991 — The upgrade algorithm in PR 7 describes two approaches for updating the scaffold shim ref: step 5 says "Regenerate scaffold shim with new ref using scaffold.PerRepoShimTemplate()" (full template regeneration), while below it defines replaceShimRef() for in-place regex replacement. These are complementary strategies, but the plan doesn't clarify which is preferred or when each applies.
    Remediation: Clarify which approach is preferred, or note that replaceShimRef is a fallback for cases where template regeneration isn't appropriate.

  • [internal-consistency] docs/plans/repos-management.md:1333 — The dependency graph declares "PR 7 depends on PR 6 (upgrade checks for drift before proceeding)" but the upgrade algorithm never explicitly calls Diff() or Status() from PR 6. The dependency may exist for code reuse of discovery patterns, but the stated justification doesn't match the algorithm.
    Remediation: Clarify what specific PR 6 functionality PR 7 reuses, or adjust the dependency graph.

  • [internal-consistency] docs/plans/repos-management.md:2090repos remove lacks the explicit three-phase parallel/sequential structure that repos install has. The WIF deregistration is marked sequential, and per-repo deletions are implicitly parallelizable, but the execution model isn't formally specified.

  • [broken-forward-reference] docs/ADRs/0049-repos-management.md:714 — ADR 0049 references "ADR 0044 (deprecate per-org installation mode, PR docs(adr): deprecate per-org installation mode #2340)" multiple times but ADR 0044 does not exist in the repository. The references are consistently marked as "pending" and the ADR explicitly states it "can be built and shipped independently of ADR 0044."

  • [edge-case] docs/ADRs/0049-repos-management.md:265 — The field resolution rule "Empty-string and zero-value overrides are treated as unset and fall through to defaults" makes it impossible to explicitly clear a field for one repo without removing the default for all repos.

Info

  • [forward-reference-clarity] docs/ADRs/0049-repos-management.md:48 — ADR 0049 "builds on" ADR 0048 and states version-management commands "require its --upstream-ref mechanism," but ADR 0048 describes a high-level decision rather than implementation details like __FULLSEND_REF__ template variables.

  • [scope-creep-signal] docs/ADRs/0049-repos-management.md:897 — The "Future enhancements" section proposes a "Unified install command" where repos install subsumes github setup — a significant UX shift that would benefit from its own ADR when pursued.

  • [documentation-coherence] docs/architecture.md:955 — Repos management placed in "Agent Infrastructure" section. The section covers provisioning and lifecycle management so the placement is defensible, though repos management is primarily CLI orchestration for installation.

  • [adr-reference-consistency] docs/ADRs/0049-repos-management.md:942 — In the References section, ADR 0044 is listed as plain text without a markdown link, while all other ADR references use relative path links. The (PR #2340, pending) annotation explains the missing link.

Previous run (15)

Review

Findings

Medium

  • [internal-consistency] docs/plans/repos-management.md:202 — PR 5 (batch install) Phase 2 performs WIF provisioning and mint registration sequentially, then Phase 3 calls Install() from PR 1 in parallel. However, the InstallConfig struct defined in PR 1 has no field to supply a pre-provisioned WIF provider. The existing runPerRepoInstall performs WIF provisioning internally. Without a mechanism for Install() to accept an already-provisioned WIF provider and skip re-provisioning, Phase 3 would either duplicate the WIF work from Phase 2 or fail due to double-registration. The text says "Call Install() (from PR 1) with the provisioned WIF provider" but no such parameter exists.
    Remediation: Add a WIFProvider string field to InstallConfig (or equivalent skip flag). When non-empty, Install() should skip ProvisionWIF, RegisterPerRepoWIF, and EnsureOrgInMint and use the provided value directly. Alternatively, document that BatchInstall calls scaffold/variable-write logic directly instead of the full Install() function.

Low

  • [broken-forward-reference] docs/ADRs/0049-repos-management.md:722 — ADR 0049 references "ADR 0044 (deprecate per-org installation mode, PR docs(adr): deprecate per-org installation mode #2340)" multiple times but ADR 0044 does not exist in the repository (numbering jumps from 0043 to 0045). The references are self-documenting — marked "(PR docs(adr): deprecate per-org installation mode #2340, pending)" — and the ADR's core decision explicitly does not depend on 0044 ("can be built and shipped independently").

  • [edge-case] docs/ADRs/0049-repos-management.md:271 — Glob expansion uses filepath.Match for filtering. The ADR says "glob pattern" without restricting to org-level wildcards, but the only example is acme-oss/* and the implementation plan restricts validation to "single * at end of org prefix". This creates ambiguity about whether patterns like org/prefix-* are valid.
    Remediation: Clarify in the schema section whether glob patterns are restricted to org-level wildcards (org/*) or support repo-name patterns (org/prefix-*).

  • [internal-consistency] docs/plans/repos-init.md:160 — Manifest generation says mint URL can be "derived from --mint-project" but there is no --mint-url flag in the flags list, and Cloud Run URLs include a random hash that cannot be predicted from the project name alone.
    Remediation: Add a --mint-url flag, specify a DiscoverMint call using --mint-project and --mint-region, or remove the "derived from --mint-project" phrasing.

Info

  • [missing-authorization] — No linked issue for this non-trivial PR. ADRs are themselves decision documents and this is documentation-only, so this is a process note rather than a blocker.

  • [adr-reference-consistency] docs/ADRs/0049-repos-management.md:942 — In the References section, ADR 0044 is listed as plain text without a markdown link, while all other ADR references use relative path links. The (PR #2340, pending) annotation explains the missing link.

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread docs/ADRs/0051-repos-management.md
Comment thread internal/cli/run_test.go Outdated
Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/ADRs/0051-repos-management.md
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:44 PM UTC · Completed 5:56 PM UTC
Commit: eb4e66d · View workflow run →

Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/ADRs/0051-repos-management.md
@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 17, 2026
Comment thread docs/ADRs/0049-repos-management.md Outdated
Comment thread docs/ADRs/0051-repos-management.md
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:43 PM UTC · Completed 10:56 PM UTC
Commit: 2500322 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed ready-for-merge All reviewers approved — ready to merge labels Jun 17, 2026
@ggallen ggallen force-pushed the worktree-adr-fleet-management branch from 2500322 to e46f72d Compare June 17, 2026 22:58
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:02 PM UTC · Completed 11:14 PM UTC
Commit: e46f72d · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 17, 2026
@ggallen ggallen force-pushed the worktree-adr-fleet-management branch from e46f72d to 812cbc0 Compare June 17, 2026 23:16
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:20 PM UTC · Completed 11:34 PM UTC
Commit: 812cbc0 · View workflow run →

Comment thread docs/ADRs/0049-repos-management.md Outdated
Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/ADRs/0051-repos-management.md
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 17, 2026
@ggallen ggallen force-pushed the worktree-adr-fleet-management branch from 812cbc0 to c40dcfe Compare June 17, 2026 23:39
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 18, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:56 AM UTC · Completed 4:10 AM UTC
Commit: 4a7ba40 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 18, 2026
Comment thread docs/ADRs/0049-repos-management.md Outdated
Comment thread docs/ADRs/0049-repos-management.md Outdated
Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/ADRs/0051-repos-management.md
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 18, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:53 AM UTC · Completed 12:08 PM UTC
Commit: 211a5c8 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 18, 2026

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Squad — Multi-Agent Analysis (6 agents: Claude, Gemini, Codex)

10 findings posted inline (4 HIGH, 6 MEDIUM). See inline comments for details.

Full report with LOW/INFO findings available on request.

Comment thread docs/ADRs/0049-repos-management.md Outdated
Comment thread docs/ADRs/0049-repos-management.md Outdated
Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/ADRs/0049-repos-management.md Outdated
Comment thread docs/ADRs/0049-repos-management.md Outdated
Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/plans/repos-management.md Outdated
Comment thread docs/plans/repos-management.md
Comment thread docs/plans/repos-management.md
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 19, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:35 AM UTC · Completed 12:50 AM UTC
Commit: 7b2856b · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread docs/architecture.md
Comment thread docs/architecture.md
Comment thread docs/architecture.md Outdated
Comment thread docs/plans/repos-management.md
Comment thread docs/ADRs/0051-repos-management.md
Comment thread docs/plans/repos-management.md
Comment thread docs/ADRs/0051-repos-management.md
@fullsend-ai-review fullsend-ai-review Bot added component/docs User-facing documentation component/install CLI install and app setup and removed requires-manual-review Review requires human judgment labels Jun 19, 2026
…stallations

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

fullsend-ai-review Bot commented Jun 19, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:48 AM UTC · Completed 12:08 PM UTC
Commit: 2da14dd · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 19, 2026

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structural feedback on ADR scope

Thanks for addressing the round-1 findings — all 10 were handled cleanly.

After a second pass with fresh eyes, the main concern is structural rather than content bugs: the ADR is ~975 lines, which is roughly 25x the repo's own ADR template (0000-adr-template.md). ADR 0001 says problem docs are for open-ended exploration while ADRs are for crystallizing specific decisions — and the template intentionally has no implementation section.

A few recommendations:

1. Slim the ADR to the decision layer (~400 lines)
The core decision — declarative repos.yaml manifest, repos subcommand group for platform admins, three-phase execution model — is solid. But the ADR currently duplicates content that already lives in docs/plans/repos-management.md and docs/plans/repos-init.md: sync reconciliation tables, replaceShimRef() regex details, WIFProvisioner interface shapes, PR-level dependency graphs. Per the repo's conventions, that detail belongs in the plans, with the ADR referencing them.

2. Resolve contradictions between ADR and plans
Having the same detail in two places has already caused drift:

  • ADR sync scope text (lines 270-272) says base_harness and allowed_remote_resources are reconciled, but the sync table (lines 582-587) only lists 4 resources and doesn't include them
  • Plan PR 5 header (line 551) says "Depends on: PR 1, PR 2" but the summary (line 39) correctly says "PR 5 depends on PRs 1, 2, and 3"
  • FULLSEND_ALLOWED_RESOURCES is referenced (ADR line 268) but doesn't exist in the codebase

Keeping implementation detail in one place (the plans) avoids this.

3. Gate version management on ADR 0048
The upgrade and upgrade-mint commands, plus replaceShimRef() in the plan, target field names (fullsend_actions_ref, fullsend_cli_ref) that don't exist yet — they're proposed in ADR 0048 which is still marked "assumed implemented" (line 992). Recommend either deferring PRs 7-8 until ADR 0048 ships, or splitting them into a separate follow-up ADR/plan that explicitly depends on 0048.

4. Consider splitting the PR
ADR in one PR, plans in a follow-up after the ADR decision is accepted. This makes the ADR reviewable on its own merits without the plans creating additional surface area.

None of these are blockers if the team prefers to keep the current structure, but slimming the ADR to match the repo's conventions would make it easier to review and maintain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/docs User-facing documentation component/install CLI install and app setup ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants