refactor(forge)!: prepare interface for GitLab support

[ggallen]

Summary

• Adds ErrNotSupported sentinel and PipelineSchedule type to the forge package
• Adds 6 new methods to forge.Client: IsProtectedBranch, CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable
• Extracts ListOrgInstallations and GetAppClientID from forge.Client into a new GitHubExtensions interface, updating all callers (appsetup, cli) to use type-assertion
• Implements IsProtectedBranch on GitHub via the branch protection API; stubs pipeline/variable methods with ErrNotSupported
• Supports gitlab.com in IsSupportedForge and ParseForgeURL (including nested groups via /-/ separator)
• Adds FakeClient implementations for all new methods with configurable fields and call recorders
• Adds tests for IsProtectedBranch, ErrNotSupported stubs, and GitLab URL parsing

This is Phase 0 of GitLab forge support: pure refactoring + interface evolution with no behavioral changes to existing functionality.

Test plan

• go build ./... passes
• go vet ./... passes
• go test ./internal/forge/... ./internal/appsetup/... ./internal/cli/... all pass
• make lint passes
• New tests cover IsProtectedBranch (protected + not protected), ErrNotSupported stubs, and GitLab URL parsing (simple group, nested groups, root path, missing separator)

Breaking changes

ListOrgInstallations and GetAppClientID are removed from forge.Client and moved to the new GitHubExtensions interface. All in-tree callers are updated. Out-of-tree callers (if any) must type-assert to forge.GitHubExtensions.

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:01 AM UTC · Completed 12:19 AM UTC
Commit: 3aaf789 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

forge: evolve Client interface for multi-forge + GitLab URL parsing

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Evolve forge.Client to support cross-forge capabilities and explicit unsupported operations.
• Split GitHub-only app-installation APIs into GitHubExtensions and update callers via type
 assertion.
• Add GitLab URL parsing support and tests for new forge behaviors.

Diagram

graph TD
  A["appsetup"] --> B["forge.Client"] --> D["github.LiveClient"]
  C["cli"] --> K["ParseForgeURL"] --> B
  B --> E["forge.FakeClient"]
  A -. "type assert" .-> F["forge.GitHubExtensions"]
  C -. "type assert" .-> F
  D --> F
  E --> F

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Capability-specific interfaces (BranchProtection/Schedules/Variables/AppInstalls)

• ➕ Avoids adding methods to the base Client that many forges will stub
• ➕ Call sites can cleanly gate behavior via type assertion per capability
• ➕ Reduces the surface area of ErrNotSupported checks
• ➖ More interfaces/types to manage and document
• ➖ Potentially more verbose call sites (multiple assertions)
• ➖ Harder to discover 'what this forge supports' at a glance

2. Client.Capabilities() discovery + optional methods

• ➕ Centralizes support checks without type assertions everywhere
• ➕ Allows consistent UX messaging (feature unavailable) at call sites
• ➖ Still requires careful runtime gating before calling methods
• ➖ Can drift if Capabilities and implementation get out of sync

3. Per-forge higher-level adapters (GitHubClient/GitLabClient) behind a factory

• ➕ Keeps forge-specific APIs out of the shared abstraction entirely
• ➕ Avoids stubbing unrelated methods on implementations
• ➖ Larger refactor for existing code that expects a single Client type
• ➖ More complex wiring and dependency injection

Recommendation: For Phase 0, the current approach is a pragmatic step: keep a single cross-forge Client, explicitly signal unsupported ops via ErrNotSupported, and isolate GitHub-app installation calls behind GitHubExtensions. As GitLab support lands and more features diverge, consider splitting CI/CD schedules and variables into capability interfaces to reduce long-term stub surface area on non-supporting forges.

Files changed (9) +426 / -28

Enhancement (3) +151 / -5

forge.goEvolve forge.Client and add GitHubExtensions + ErrNotSupported +33/-3

Evolve forge.Client and add GitHubExtensions + ErrNotSupported

• Adds ErrNotSupported sentinel and expands Client with branch protection, pipeline schedules, and variables APIs. Introduces PipelineSchedule type and extracts GitHub app-installation APIs into a separate GitHubExtensions interface (breaking change).

internal/forge/forge.go

github.goImplement IsProtectedBranch on GitHub; stub GitLab-oriented methods +40/-1

Implement IsProtectedBranch on GitHub; stub GitLab-oriented methods

• Adds a GitHub implementation of IsProtectedBranch using the branch protection endpoint (404 => not protected). Implements pipeline schedule and variable methods as ErrNotSupported stubs and declares GitHubExtensions support.

internal/forge/github/github.go

url.goSupport gitlab.com URLs (/-/ separator) in ParseForgeURL +78/-1

Support gitlab.com URLs (/-/ separator) in ParseForgeURL

• Extends supported forge hosts to include gitlab.com and adds GitLab-specific parsing that supports nested groups. Introduces hostnameToForge mapping and routes GitLab URLs to parseGitLabURL.

internal/forge/url.go

Refactor (3) +69 / -21

appsetup.goType-assert GitHubExtensions for GitHub app-installation operations +37/-16

Type-assert GitHubExtensions for GitHub app-installation operations

• Replaces direct calls to Client.GetAppClientID/ListOrgInstallations with a GitHubExtensions type assertion. Introduces ErrNotSupported/ErrNotFound-based behavior when the client is not GitHub-capable.

internal/appsetup/appsetup.go

admin.goGate installation listing behind GitHubExtensions in admin flows +25/-4

Gate installation listing behind GitHubExtensions in admin flows

• Updates shared-role detection and uninstall verification to type-assert forge.GitHubExtensions before listing installations. Returns ErrNotSupported (or no-op behavior) for non-GitHub forges.

internal/cli/admin.go

github.goGitHub uninstall flow uses GitHubExtensions for installations +7/-1

GitHub uninstall flow uses GitHubExtensions for installations

• Switches installation listing to use forge.GitHubExtensions when available and returns ErrNotSupported otherwise. Preserves existing behavior for GitHub clients while enabling future non-GitHub clients.

internal/cli/github.go

Tests (3) +206 / -2

fake.goExtend FakeClient with new forge.Client methods and recorders +100/-1

Extend FakeClient with new forge.Client methods and recorders

• Adds protected-branch state, pipeline schedule storage, and variable operation recorders to FakeClient. Implements IsProtectedBranch, pipeline schedule methods, and variable methods for testability, and declares conformance to both Client and GitHubExtensions.

internal/forge/fake.go

github_test.goAdd tests for IsProtectedBranch and ErrNotSupported stubs +56/-0

Add tests for IsProtectedBranch and ErrNotSupported stubs

• Adds HTTP-driven tests validating IsProtectedBranch behavior for protected vs unprotected branches. Adds a unit test ensuring GitHub client returns ErrNotSupported for schedule/variable methods.

internal/forge/github/github_test.go

url_test.goAdd GitLab ParseForgeURL/IsSupportedForge test coverage +50/-1

Add GitLab ParseForgeURL/IsSupportedForge test coverage

• Adds table-driven tests for GitLab tree/blob URLs, nested groups, root-path parsing, and missing /-/ separator errors. Updates IsSupportedForge tests to expect gitlab.com support.

internal/forge/url_test.go

[github-actions]

Site preview

Preview: https://00f1afb5-site.fullsend-ai.workers.dev

Commit: 0c7da2f3a1296bcd7367f78d3eb2ac8d381f25ae

[codecov]

Codecov Report

❌ Patch coverage is 86.38498% with 29 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/forge/fake.go	80.72%	8 Missing and 8 partials ⚠️
internal/forge/url.go	81.39%	5 Missing and 3 partials ⚠️
internal/harness/harness.go	0.00%	4 Missing ⚠️
internal/forge/github/github.go	95.23%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. Unescaped branch in URL ✓ Resolved 🐞 Bug ≡ Correctness

Description

github.LiveClient.IsProtectedBranch interpolates the branch name directly into the REST path without
URL-escaping, so branches containing '/' or other reserved characters will call the wrong endpoint
and can be misclassified as unprotected. This can break any logic that relies on accurate
branch-protection detection.

Code

internal/forge/github/github.go[R2987-2995]

+func (c *LiveClient) IsProtectedBranch(ctx context.Context, owner, repo, branch string) (bool, error) {
+	_, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/branches/%s/protection", owner, repo, branch))
+	if err != nil {
+		if isNotFound(err) {
+			return false, nil
+		}
+		return false, fmt.Errorf("check branch protection: %w", err)
+	}
+	return true, nil

Relevance

⭐⭐ Medium

No clear historical precedent on URL-escaping branch names; GitHub client correctness issues are
sometimes fixed, sometimes deferred.

PR-#816
PR-#2201

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new method builds the request path using the raw branch value, while other parts of the GitHub
client explicitly URL-escape path/query components before calling c.get, demonstrating that
escaping is required and expected at the call site.

internal/forge/github/github.go[2985-2995]
internal/forge/github/github.go[282-291]
internal/forge/github/github.go[1146-1162]

Agent prompt

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

### Issue description
`LiveClient.IsProtectedBranch` builds `"/repos/%s/%s/branches/%s/protection"` using the raw `branch` string. Branch names commonly contain `/` (e.g., `release/1.2`), which must be URL-escaped when used as a single path segment, otherwise the request path is split into multiple segments and returns incorrect results.

### Issue Context
The GitHub client already contains helpers and examples that escape path parameters (e.g., `escapePathSegments`, `url.PathEscape`, `url.QueryEscape`), and `c.get` expects callers to pass a correctly escaped path.

### Fix Focus Areas
- internal/forge/github/github.go[2987-2995]

### Suggested fix
- Build the path using `url.PathEscape` for `owner`, `repo`, and especially `branch`:
 - `fmt.Sprintf("/repos/%s/%s/branches/%s/protection", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(branch))`
- Add a unit test case for a branch name containing `/` (ensure the server sees the escaped path, e.g. `feature%2Ffoo`).

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

---

2. Fake schedules state mismatch ✓ Resolved 🐞 Bug ≡ Correctness

Description

FakeClient.CreatePipelineSchedule appends only to CreatedSchedules, but ListPipelineSchedules
returns only the PipelineSchedules map, so a created schedule is not visible via list (and
DeletePipelineSchedule cannot remove anything from list). This makes FakeClient behavior internally
inconsistent and can cause tests to pass/fail incorrectly once schedule workflows are introduced.

Code

internal/forge/fake.go[R1579-1619]

+func (f *FakeClient) CreatePipelineSchedule(_ context.Context, owner, repo, ref, description, cron string) (int64, error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	if e := f.err("CreatePipelineSchedule"); e != nil {
+		return 0, e
+	}
+
+	s := PipelineSchedule{
+		ID:          int64(len(f.CreatedSchedules) + 1),
+		Description: description,
+		Ref:         ref,
+		Cron:        cron,
+		Active:      true,
+	}
+	f.CreatedSchedules = append(f.CreatedSchedules, s)
+	return s.ID, nil
+}
+
+func (f *FakeClient) DeletePipelineSchedule(_ context.Context, _, _ string, scheduleID int64) error {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	if e := f.err("DeletePipelineSchedule"); e != nil {
+		return e
+	}
+
+	f.DeletedScheduleIDs = append(f.DeletedScheduleIDs, scheduleID)
+	return nil
+}
+
+func (f *FakeClient) ListPipelineSchedules(_ context.Context, owner, repo string) ([]PipelineSchedule, error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	if e := f.err("ListPipelineSchedules"); e != nil {
+		return nil, e
+	}
+
+	return f.PipelineSchedules[owner+"/"+repo], nil
+}

Relevance

⭐⭐⭐ High

FakeClient behavior consistency is enforced historically; create/list state mismatches are usually
fixed when introduced.

PR-#1215
PR-#689
PR-#3001

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The create/list/delete implementations clearly use different storage, so a schedule created via
CreatePipelineSchedule cannot be returned by ListPipelineSchedules. This differs from the
established FakeClient pattern where created entities can be retrieved by subsequent read calls.

internal/forge/fake.go[1579-1619]
internal/forge/fake.go[273-325]

Agent prompt

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

### Issue description
`FakeClient`’s pipeline schedule methods don’t share a single source of truth:
- `CreatePipelineSchedule` records into `CreatedSchedules` only.
- `ListPipelineSchedules` reads from `PipelineSchedules` only.
- `DeletePipelineSchedule` records IDs but does not update `PipelineSchedules`.

This means `Create` does not affect `List`, which is surprising and inconsistent with other FakeClient behaviors.

### Issue Context
Other FakeClient operations keep created state visible to reads (e.g., `CreateRepo` updates `CreatedRepos`, and `GetRepo` checks both `Repos` and `CreatedRepos`).

### Fix Focus Areas
- internal/forge/fake.go[1579-1619]

### Suggested fix
- In `CreatePipelineSchedule`, also append the created schedule into `PipelineSchedules[owner+"/"+repo]`.
- In `DeletePipelineSchedule`, remove the schedule from `PipelineSchedules[owner+"/"+repo]` by ID (you may need owner/repo params, so don’t discard them with `_`).
- Keep `CreatedSchedules` / `DeletedScheduleIDs` as call recorders if they’re useful, but make `ListPipelineSchedules` reflect current state.

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

---

3. Appsetup/CLI changes untested 📘 Rule violation ▣ Testability

Description

Logic was modified in internal/appsetup/appsetup.go and internal/cli/admin.go (new
GitHubExtensions type-assertion paths and new error behaviors), but this PR does not add or update
unit tests in the corresponding packages to exercise these new paths. This increases the risk of
regressions for non-GitHub forges or callers that do not implement forge.GitHubExtensions.

Code

internal/appsetup/appsetup.go[R266-272]

+	var clientID string
+	var lookupErr error
+	if ghExt, ok := s.client.(forge.GitHubExtensions); ok {
+		clientID, lookupErr = ghExt.GetAppClientID(ctx, slug)
+	} else {
+		lookupErr = forge.ErrNotFound
+	}

Relevance

⭐⭐⭐ High

Appsetup/CLI logic changes usually require updating tests; similar appsetup refactors added tests in
PRs.

PR-#1019
PR-#1104
PR-#2164

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062049 requires tests for new or modified Go logic. The cited hunks introduce new
conditional branches (forge.GitHubExtensions type assertions and fallback error behavior) without
corresponding package-level unit test updates in this change set.

Rule 1062049: Require tests for new or modified Go logic
internal/appsetup/appsetup.go[266-272]
internal/cli/admin.go[1382-1386]

Agent prompt

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

## Issue description
This PR changes behavior in app setup and CLI flows by type-asserting `forge.Client` to `forge.GitHubExtensions` and returning different errors/fallbacks when the assertion fails, but no unit tests were added/updated in those packages to cover the new branches.

## Issue Context
Compliance requires tests for new or modified Go logic unless an explicit exception is documented.

## Fix Focus Areas
- internal/appsetup/appsetup.go[266-272]
- internal/cli/admin.go[1382-1386]

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

---

4. FakeClient maps not initialized ✓ Resolved 🐞 Bug ☼ Reliability

Description

FakeClient adds map fields for ProtectedBranches and PipelineSchedules, but NewFakeClient does not
initialize them, so callers using NewFakeClient() and then assigning into these maps will panic with
"assignment to entry in nil map". This violates NewFakeClient’s stated contract that it returns a
FakeClient with all maps initialized.

Code

internal/forge/fake.go[R152-157]

+	// Protected branches for IsProtectedBranch.
+	ProtectedBranches map[string]bool // key: "owner/repo/branch"
+
+	// Pipeline schedules for List/Create/DeletePipelineSchedule.
+	PipelineSchedules map[string][]PipelineSchedule // key: "owner/repo"
+

Relevance

⭐⭐⭐ High

Repo frequently fixes FakeClient contract/consistency issues; such reliability findings are
typically accepted.

PR-#689
PR-#1215
PR-#3001

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds two new map fields, but NewFakeClient’s constructor does not initialize them. The
codebase already uses NewFakeClient and then assigns into initialized maps (e.g., BranchRefs) which
would similarly be expected for the new maps.

internal/forge/fake.go[152-157]
internal/forge/fake.go[14-27]
internal/cli/agent_test.go[665-667]

Agent prompt

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

### Issue description
`FakeClient` gained new map fields (`ProtectedBranches`, `PipelineSchedules`), but `NewFakeClient()` doesn’t initialize them. Any test/fixture that follows the common pattern `fc := forge.NewFakeClient(); fc.<Map>[k] = v` will panic when using these new maps.

### Issue Context
`NewFakeClient` is documented as returning a FakeClient with all maps initialized, and the repo already relies on this pattern for other maps.

### Fix Focus Areas
- internal/forge/fake.go[14-27]
- internal/forge/fake.go[152-157]

### Suggested fix
- In `NewFakeClient()`, add:
 - `ProtectedBranches: make(map[string]bool)`
 - `PipelineSchedules: make(map[string][]PipelineSchedule)`
- Keep the existing “all maps initialised” guarantee accurate (either initialize all map fields or adjust the comment).

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

---

View more (1)

5. Unsupported mapped to notfound ✓ Resolved 🐞 Bug ☼ Reliability

Description

appsetup.Setup.Run forces lookupErr=forge.ErrNotFound when the client doesn’t implement
forge.GitHubExtensions, which drives the flow as if the app “doesn’t exist” rather than “cannot be
checked”. This delays the eventual ErrNotSupported failure (e.g., in ensureInstalled) and can
trigger misleading/incorrect control-flow for non-GitHub clients.

Code

internal/appsetup/appsetup.go[R266-272]

+	var clientID string
+	var lookupErr error
+	if ghExt, ok := s.client.(forge.GitHubExtensions); ok {
+		clientID, lookupErr = ghExt.GetAppClientID(ctx, slug)
+	} else {
+		lookupErr = forge.ErrNotFound
+	}

Relevance

⭐⭐ Medium

Team cares about correct sentinel error semantics, but no prior example of ErrNotFound vs
ErrNotSupported mapping in appsetup.

PR-#2595
PR-#2201

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Setup.Run explicitly maps missing GitHubExtensions to ErrNotFound, while other parts of the same
Setup hard-error with ErrNotSupported when GitHubExtensions is required, proving this is a semantic
mismatch that changes control-flow and delays failures.

internal/appsetup/appsetup.go[261-275]
internal/appsetup/appsetup.go[382-386]
internal/appsetup/appsetup.go[869-875]

Agent prompt

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

### Issue description
In `Setup.Run`, absence of `forge.GitHubExtensions` is converted into `forge.ErrNotFound`, which is semantically wrong (unsupported != not found) and causes the setup flow to proceed into “create new app” paths before failing later when installations are needed.

### Issue Context
The same Setup now returns `ErrNotSupported` in other GitHubExtensions-gated operations (e.g., `ensureInstalled`), so failing fast at the first required GitHub-only capability is more consistent and avoids wasted work and confusing UX.

### Fix Focus Areas
- internal/appsetup/appsetup.go[266-275]
- internal/appsetup/appsetup.go[382-386]
- internal/appsetup/appsetup.go[869-875]

### Suggested fix
- When `s.client` is not a `forge.GitHubExtensions`, return an error wrapping `forge.ErrNotSupported` instead of setting `lookupErr = forge.ErrNotFound`.
- Consider making `findExistingInstallation` return an `ErrNotSupported` error as well (instead of `nil, false, nil`) so unsupported clients fail deterministically and early.

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

---

[fullsend-ai-review]

Review

Clean Phase 0 forge interface preparation — the only changes since the prior review at 3c1558b are fixes to the two stale-reference findings: schedule ID types in the implementation plan updated from string to int64, and ADR-0067 line 401 updated from UpdateVariable to UpdateCIVariable. Both fixes are correct and complete.

All type-assertion patterns across appsetup.go, admin.go, github.go remain fail-closed: Run() rejects non-GitHub clients before reaching downstream methods; handleExistingApp() and ensureInstalled() return errors; recoverCreatedApp() and findExistingInstallation() return nil gracefully (only reachable from Run() which already guards). detectSharedApps() returns fallback data. Security analysis found no fail-open paths. The IsProtectedBranch implementation correctly uses strings.EqualFold for the 404 message comparison and url.PathEscape for all path segments, with proper test coverage including branch names with slashes. All fetch/resolve paths that call ParseForgeURL have explicit info.Forge != "github" guards preventing premature GitLab URL handling.

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateCIVariable, CreateProtectedCIVariable) have no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. The breaking change is correctly marked with ! in the PR title, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions and tested for non-GitHub fallback behavior. Severity anchored from prior review.

Low

• [missing-authorization] — This non-trivial refactoring PR (840 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067 (unchanged)
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + internal/ scope
missing-authorization (no linked issue)	Low	🔄 Persists — authorized via ADRs
stale-reference (plan schedule ID types)	Low	✅ Fixed — CreatePipelineSchedule returns (int64, error) and DeletePipelineSchedule accepts scheduleID int64
stale-reference (ADR-0067 UpdateVariable)	Low	✅ Fixed — line 401 now references UpdateCIVariable

Previous run

Review

Clean Phase 0 forge interface preparation — the only change since the prior review at 5fa1dbf is a rebase (single commit, new SHA 3c1558b). The prior review's stale-reference finding about old method names (UpdateVariable/CreateProtectedVariable) in the implementation plan is fully fixed: all references now use UpdateCIVariable/CreateProtectedCIVariable with correct signatures including the protected bool parameter.

All type-assertion patterns across appsetup.go, admin.go, github.go remain fail-closed: Run() rejects non-GitHub clients before reaching downstream methods; handleExistingApp() and ensureInstalled() return errors; recoverCreatedApp() and findExistingInstallation() return nil gracefully (only reachable from Run() which already guards). detectSharedApps() returns fallback data. Security analysis found no fail-open paths. The IsProtectedBranch implementation correctly uses strings.EqualFold for the 404 message comparison and url.PathEscape for all path segments, with proper test coverage including branch names with slashes. All fetch/resolve paths that call ParseForgeURL have explicit info.Forge != "github" guards preventing premature GitLab URL handling.

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateCIVariable, CreateProtectedCIVariable) have no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. The breaking change is correctly marked with ! in the PR title, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions and tested for non-GitHub fallback behavior. Severity anchored from prior review.

Low

• [missing-authorization] — This non-trivial refactoring PR (837 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

• [stale-reference] docs/plans/gitlab-cron-polling-implementation.md — The implementation plan's interface spec (lines 39–40) shows CreatePipelineSchedule returning (scheduleID string, err error) and DeletePipelineSchedule accepting scheduleID string, but the actual forge interface uses int64 for schedule IDs. The PR correctly updated the variable method names and signatures in the plan but missed this type mismatch for the schedule methods. Developers implementing future phases would encounter compile errors.
  Remediation: Update the plan's interface spec to match: CreatePipelineSchedule(...) (int64, error) and DeletePipelineSchedule(ctx context.Context, owner, repo string, scheduleID int64) error.

• [stale-reference] docs/ADRs/0067-gitlab-cron-polling-event-dispatch.md — Line 401 references UpdateVariable (old forge method name), which has been renamed to UpdateCIVariable in this PR. The ADR is Accepted, so only minor annotations are appropriate per AGENTS.md policy.
  Remediation: Update line 401 to reference UpdateCIVariable.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067 (method names updated)
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + internal/ scope
missing-authorization (no linked issue)	Low	🔄 Persists — authorized via ADRs
stale-reference (plan method names)	Low	✅ Fixed — all method names and signatures updated in implementation plan

Previous run (2)

Review

Clean Phase 0 forge interface preparation — the method rename from UpdateVariable/CreateProtectedVariable to UpdateCIVariable/CreateProtectedCIVariable is the only change since the prior review at c5cb18f. The rename improves naming clarity by disambiguating from the existing RepoVariable methods (GitHub Actions variables vs GitLab CI protected variables). All prior-review fixes remain in place: the info.Forge != "github" guards in resolve.go, fetchsvc/service.go, agent.go, and harness.go; the updated comments in detect.go/detect_test.go; and the dead-code fix in admin.go runUninstall.

All type-assertion patterns across appsetup.go, admin.go, github.go are fail-closed: Run() rejects non-GitHub clients before reaching downstream methods; handleExistingApp() and ensureInstalled() return errors; recoverCreatedApp() and findExistingInstallation() return nil gracefully (only reachable from Run() which already guards). detectSharedApps() returns fallback data. Security analysis found no fail-open paths. The IsProtectedBranch implementation correctly uses strings.EqualFold for the 404 message comparison and url.PathEscape for all path segments, with proper test coverage including branch names with slashes.

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateCIVariable, CreateProtectedCIVariable) have no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. The breaking change is correctly marked with ! in the PR title, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions and tested for non-GitHub fallback behavior. Severity anchored from prior review.

Low

• [missing-authorization] — This non-trivial refactoring PR (818 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

• [stale-reference] docs/plans/gitlab-cron-polling-implementation.md — The implementation plan references the old method names UpdateVariable and CreateProtectedVariable throughout (lines 42–43, 209–210, 1064, 1096, 1676–1701, 1819), but the forge interface now uses UpdateCIVariable and CreateProtectedCIVariable. The plan's signatures also omit the protected bool parameter on UpdateCIVariable. This plan guides future phases; developers following it would encounter compile errors.
  Remediation: Update method names and signatures in the implementation plan to match the implemented interface.

Prior review resolution

Prior finding	Severity	Status
consumer-completeness (resolve.go, service.go)	Medium	✅ Fixed — both callers have info.Forge != "github" guard
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067 (method names updated)
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + internal/ scope
missing-authorization (no linked issue)	Low	🔄 Persists — authorized via ADRs
stale-reference (detect.go comments)	Low	✅ Fixed — comments updated to reflect isRecognizedForge
dead-code (admin.go ghOK re-check)	Low	✅ Fixed — verify block uses ghExt directly

Previous run (3)

Review

Clean Phase 0 forge interface preparation — the prior review's actionable consumer-completeness finding has been fixed. Both resolve.go and fetchsvc/service.go now include the info.Forge != "github" guard after ParseForgeURL, with tests covering the new rejection path. The stale-reference finding in detect.go/detect_test.go is resolved (comments updated). The dead-code finding in admin.go runUninstall is resolved (verification block uses ghExt directly, unreachable when ghOK is false).

All type-assertion patterns across appsetup.go, admin.go, github.go are fail-closed: Run() rejects non-GitHub clients before reaching downstream methods; handleExistingApp() and ensureInstalled() return errors; recoverCreatedApp() and findExistingInstallation() return nil gracefully (only reachable from Run() which already guards). detectSharedApps() returns fallback data. Security analysis found no fail-open paths. The IsProtectedBranch implementation correctly uses strings.EqualFold for the 404 message comparison and url.PathEscape for all path segments, with proper test coverage including branch names with slashes.

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) have no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. The breaking change is correctly marked with ! in the PR title, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions and tested for non-GitHub fallback behavior. Severity anchored from prior review.

Low

• [missing-authorization] — This non-trivial refactoring PR (806 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

Prior review resolution

Prior finding	Severity	Status
consumer-completeness (resolve.go, service.go)	Medium	✅ Fixed — both callers now have info.Forge != "github" guard
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + internal/ scope
missing-authorization (no linked issue)	Low	🔄 Persists — authorized via ADRs
stale-reference (detect.go comments)	Low	✅ Fixed — comments updated to reflect isRecognizedForge
dead-code (admin.go ghOK re-check)	Low	✅ Fixed — verify block uses ghExt directly

Previous run (4)

Review

Clean Phase 0 forge interface preparation. All prior review findings persist with no regressions. The type-assertion patterns across appsetup.go, admin.go, and github.go are correct and fail-closed — security analysis found no fail-open paths. The IsProtectedBranch implementation correctly disambiguates "not protected" vs "not found" via the 404 message, with proper URL escaping for branch names containing slashes. Tests cover the new functionality thoroughly including non-GitHub client fallback paths across all call sites. Documentation updates in the three in-diff doc files correctly annotate the move to GitHubExtensions.

Findings

Medium

• [consumer-completeness] internal/resolve/resolve.go:313, internal/fetchsvc/service.go:129 — ParseForgeURL now accepts GitLab URLs (via isRecognizedForge), but two production callers lack the secondary forge check that harness.go:ValidateResourceTypes correctly added (info.Forge != "github"). If a GitLab URL passes allowed_remote_resources and reaches these paths, the fetch would fail at the git-fetch level with an unhelpful error instead of a clear validation message. Risk is mitigated: ValidateResourceTypes rejects GitLab skills at configuration time, and allowed_remote_resources must explicitly include gitlab.com.
  Remediation: Add if info.Forge != "github" guard after ParseForgeURL in both callers, matching the pattern in harness.go.

• [scope-creep] internal/forge/forge.go — Five stub methods on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) have no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. The breaking change is correctly marked with ! in the PR title, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions and tested for non-GitHub fallback behavior. Severity anchored from prior review.

Low

• [missing-authorization] — This non-trivial refactoring PR (768 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

• [stale-reference] internal/forge/detect.go:14, internal/forge/detect_test.go:234 — Comments state that IsSupportedForge gates ParseForgeURL support, but after this PR ParseForgeURL uses isRecognizedForge instead. The test assertion still passes (returns false for gitlab.com), but the explanatory comment is misleading.
  Remediation: Update detect.go comment to reference isRecognizedForge and update detect_test.go comment to e.g. "IsSupportedForge gates fetch support (harness validation), not URL parsing."

• [dead-code] internal/cli/admin.go — In runUninstall, the verification path re-checks ghOK and sets verifyErr = forge.ErrNotSupported when false. However, this block is inside if len(existingSlugs) > 0, which can only be true when the first ListOrgInstallations succeeded (i.e., ghOK was true). The false branch is unreachable.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + internal/ scope
missing-authorization (no linked issue)	Low	🔄 Persists — authorized via ADRs
code-organization (extra blank line)	Low	🔄 Persists — cosmetic, not addressed

Previous run (5)

Review

Clean Phase 0 forge interface preparation. All prior review findings persist with no regressions. The type-assertion patterns across appsetup.go, admin.go, and github.go are correct and fail-closed — security analysis found no fail-open paths. The IsProtectedBranch implementation correctly disambiguates "not protected" vs "not found" via the 404 message, with proper URL escaping for branch names containing slashes. Tests cover the new functionality thoroughly including non-GitHub client fallback paths across all call sites. Documentation updates in the three in-diff doc files correctly annotate the move to GitHubExtensions.

Findings

Medium

• [consumer-completeness] internal/resolve/resolve.go:313, internal/fetchsvc/service.go:129 — ParseForgeURL now accepts GitLab URLs (via isRecognizedForge), but two production callers lack the secondary forge check that harness.go:ValidateResourceTypes correctly added (info.Forge != "github"). If a GitLab URL passes allowed_remote_resources and reaches these paths, the fetch would fail at the git-fetch level with an unhelpful error instead of a clear validation message. Risk is mitigated: ValidateResourceTypes rejects GitLab skills at configuration time, and allowed_remote_resources must explicitly include gitlab.com.
  Remediation: Add if info.Forge != "github" guard after ParseForgeURL in both callers, matching the pattern in harness.go.

• [scope-creep] internal/forge/forge.go — Five stub methods on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) have no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. The breaking change is correctly marked with ! in the PR title, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions and tested for non-GitHub fallback behavior. Severity anchored from prior review.

Low

• [missing-authorization] — This non-trivial refactoring PR (768 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

• [stale-reference] internal/forge/detect.go:14, internal/forge/detect_test.go:234 — Comments state that IsSupportedForge gates ParseForgeURL support, but after this PR ParseForgeURL uses isRecognizedForge instead. The test assertion still passes (returns false for gitlab.com), but the explanatory comment is misleading.
  Remediation: Update detect.go comment to reference isRecognizedForge and update detect_test.go comment to e.g. "IsSupportedForge gates fetch support (harness validation), not URL parsing."

• [dead-code] internal/cli/admin.go — In runUninstall, the verification path re-checks ghOK and sets verifyErr = forge.ErrNotSupported when false. However, this block is inside if len(existingSlugs) > 0, which can only be true when the first ListOrgInstallations succeeded (i.e., ghOK was true). The false branch is unreachable.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + internal/ scope
missing-authorization (no linked issue)	Low	🔄 Persists — authorized via ADRs
code-organization (extra blank line)	Low	🔄 Persists — cosmetic, not addressed

---

Labels: PR modifies forge abstraction layer and Go source exclusively

Previous run (6)

Review

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) have no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. The breaking change is correctly marked with ! in the PR title, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions and tested for non-GitHub fallback behavior. Severity anchored from prior review.

Low

• [missing-authorization] — This non-trivial refactoring PR (767 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

• [code-organization] internal/forge/forge.go — Extra blank line after IsNotSupported function (double blank line before the Repository struct). The rest of the file uses single blank lines between declarations.

Previous run (7)

Review

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods remain on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. Any out-of-tree implementation of forge.Client must implement the new methods and migrate to GitHubExtensions for the moved methods. The breaking change is correctly marked with ! in the PR title and a BREAKING CHANGE: trailer in the commit message, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions.

Low

• [missing-authorization] — This non-trivial refactoring PR (721 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

Previous run (8)

Review

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) have no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. The breaking change is correctly marked with ! in the PR title, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions and tested for non-GitHub fallback behavior. Severity anchored from prior review.

Low

• [missing-authorization] — This non-trivial refactoring PR (767 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

• [code-organization] internal/forge/forge.go — Extra blank line after IsNotSupported function (double blank line before the Repository struct). The rest of the file uses single blank lines between declarations.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + internal/ scope
missing-authorization (no linked issue)	Low	🔄 Persists — authorized via ADRs

Previous run (9)

Review

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods remain on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. Any out-of-tree implementation of forge.Client must implement the new methods and migrate to GitHubExtensions for the moved methods. The breaking change is correctly marked with ! in the PR title and a BREAKING CHANGE: trailer in the commit message, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions.

Low

• [missing-authorization] — This non-trivial refactoring PR (721 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + BREAKING CHANGE: trailer
missing-authorization (no issue)	Low	🔄 Persists — authorized by ADR-0067

Previous run (10)

Review

Clean Phase 0 forge interface preparation. All actionable feedback from the prior review at 723b228 has been addressed: the URL parser now derives forge name from hostname instead of hardcoding "github", appsetup.Run() returns early with a clear error when the forge lacks GitHubExtensions instead of falling through to the manifest flow, and documentation annotations correctly reflect the interface changes. The breaking change is properly marked (! suffix + BREAKING CHANGE: trailer), all callers use consistent type-assertion patterns with appropriate fallbacks, tests cover the new functionality thoroughly (branch protection, pipeline schedule round-trip, GitLab URL parsing including nested groups), and no security issues were found.

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods remain on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. Any out-of-tree implementation of forge.Client must implement the new methods and migrate to GitHubExtensions for the moved methods. The breaking change is correctly marked with ! in the PR title and a BREAKING CHANGE: trailer in the commit message, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions.

Low

• [missing-authorization] — This non-trivial refactoring PR (588 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + BREAKING CHANGE: trailer
stale-reference (url.go Forge: "github")	Low	✅ Fixed — now uses forgeName from hostnameToForge()
missing-authorization (no issue)	Low	🔄 Persists — authorized by ADR-0067
error-handling (appsetup.go fallthrough)	Low	✅ Fixed — early return with "app setup requires GitHub" error

Previous run (11)

Review

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods are added to the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation. See also: [breaking-api] finding at this location.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. Any out-of-tree implementation of forge.Client must implement the new methods and migrate to GitHubExtensions for the moved methods. The breaking change is correctly marked with ! in the PR title, and the PR body documents the migration path. All in-tree callers are updated with type assertions. See also: [scope-creep] finding at this location.

Low

• [stale-reference] internal/forge/url.go:89 — ParseForgeURL for GitHub URLs hardcodes Forge: "github" as a string literal. The PR introduces hostnameToForge() to map hostnames to forge names, but only uses it for the branching decision — the GitHub return path bypasses it and hardcodes the string directly. Not a bug, but creates a maintenance risk if the mapping changes.

• [missing-authorization] — This non-trivial refactoring PR (585 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

• [error-handling] internal/appsetup/appsetup.go — When the forge does not support GitHubExtensions, the Run() method falls through to runManifestFlow (GitHub-specific) which will fail at ensureInstalled with ErrNotSupported. It would be more user-friendly to detect the unsupported forge earlier and provide a clear message.

Previous run (12)

Review

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods are added to the base forge.Client interface with no in-tree callers yet. Severity anchored from prior review. Authorized by ADR-0067.

Low

• [missing-field] internal/forge/fake.go — CreateProtectedVariable records a VariableRecord with Protected defaulting to false.
• [test-coverage-gap] internal/forge/fake_test.go — No test exists for FakeClient.CreateProtectedVariable.
• [docs-staleness] docs/ADRs/0044-deprecate-per-org-installation-mode.md:336,377 — References "Keep ListOrgInstallations" without noting the move to GitHubExtensions.

Previous run (13)

Review

Medium

• [stale-reference] internal/harness/harness.go:787 — Error message hardcodes (github.com) as the only supported forge.
• [scope-creep] internal/forge/forge.go — Five stub methods added. Severity anchored. Authorized by ADR-0067.

Low

• [docs-staleness] docs/plans/deprecate-per-org-install.md — References ListOrgInstallations as retained on forge.Client.
• [docs-staleness] docs/problems/gitlab-implementation.md:421-422 — Discusses moving methods as future work, but it has been implemented.

Previous run (14)

Review

High

• [breaking-change-documentation] — Commit message lacks required BREAKING CHANGE: trailer.

Medium

• [error-handling-convention] internal/forge/forge.go:66 — Missing IsNotSupported() helper.
• [error-handling-inconsistency] internal/cli/admin.go:1823 — runUninstall presents GitHub-specific URLs on non-GitHub forge.
• [scope-creep] internal/forge/forge.go — Five stub methods.
• [behavioral-change] internal/appsetup/appsetup.go:349 — Recovery loop silently skipped without log.

Previous run (15)

Review

Clean Phase 0 forge interface preparation. All actionable feedback from the prior review at 723b228 has been addressed: the URL parser now derives forge name from hostname instead of hardcoding "github", appsetup.Run() returns early with a clear error when the forge lacks GitHubExtensions instead of falling through to the manifest flow, and documentation annotations correctly reflect the interface changes. The breaking change is properly marked (! suffix + BREAKING CHANGE: trailer), all callers use consistent type-assertion patterns with appropriate fallbacks, tests cover the new functionality thoroughly (branch protection, pipeline schedule round-trip, GitLab URL parsing including nested groups), and no security issues were found.

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods remain on the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. Any out-of-tree implementation of forge.Client must implement the new methods and migrate to GitHubExtensions for the moved methods. The breaking change is correctly marked with ! in the PR title and a BREAKING CHANGE: trailer in the commit message, and the forge package is under internal/ limiting external impact. All in-tree callers are updated with type assertions.

Low

• [missing-authorization] — This non-trivial refactoring PR (588 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
breaking-api (interface change)	Medium	🔄 Persists — correctly marked with ! + BREAKING CHANGE: trailer
stale-reference (url.go Forge: "github")	Low	✅ Fixed — now uses forgeName from hostnameToForge()
missing-authorization (no issue)	Low	🔄 Persists — authorized by ADR-0067
error-handling (appsetup.go fallthrough)	Low	✅ Fixed — early return with "app setup requires GitHub" error

Previous run (16)

Review

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods are added to the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation. See also: [breaking-api] finding at this location.

• [breaking-api] internal/forge/forge.go — The forge.Client interface has a breaking change: ListOrgInstallations and GetAppClientID are removed from Client and moved to the new GitHubExtensions interface, while 6 new methods are added. Any out-of-tree implementation of forge.Client must implement the new methods and migrate to GitHubExtensions for the moved methods. The breaking change is correctly marked with ! in the PR title, and the PR body documents the migration path. All in-tree callers are updated with type assertions. See also: [scope-creep] finding at this location.

Low

• [stale-reference] internal/forge/url.go:89 — ParseForgeURL for GitHub URLs hardcodes Forge: "github" as a string literal. The PR introduces hostnameToForge() to map hostnames to forge names, but only uses it for the branching decision — the GitHub return path bypasses it and hardcodes the string directly. Not a bug, but creates a maintenance risk if the mapping changes.
  Remediation: Use Forge: forgeName instead of Forge: "github" in the GitHub return path.

• [missing-authorization] — This non-trivial refactoring PR (585 additions, breaking interface change) has no linked GitHub issue. Authorization is established via ADR-0067 and the gitlab-cron-polling implementation plan, which explicitly describe Phase 0 interface preparation.

• [error-handling] internal/appsetup/appsetup.go — When the forge does not support GitHubExtensions, the Run() method falls through to runManifestFlow (GitHub-specific) which will fail at ensureInstalled with ErrNotSupported. It would be more user-friendly to detect the unsupported forge earlier and provide a clear message, rather than running the manifest flow and failing partway through.

Prior review resolution

Prior finding	Severity	Status
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
missing-field (CreateProtectedVariable Protected: false)	Low	✅ Fixed — now sets Protected: true
test-coverage-gap (no CreateProtectedVariable test)	Low	✅ Fixed — TestFakeClient_CreateProtectedVariable added
docs-staleness (ADR-0044 references)	Low	✅ Fixed — "(now on GitHubExtensions)" annotations added

Previous run (17)

Review

Verdict: comment

Solid Phase 0 forge interface preparation. The breaking change is correctly marked (! suffix + BREAKING CHANGE: trailer), all callers are updated with consistent type-assertion fallbacks, tests cover new functionality, and no security issues were found. All prior high and medium findings from the review at 3aaf7893eb have been addressed, and the stale-reference finding from 5eb7fbb (harness error message) is also fixed — the error message now correctly lists both github.com and gitlab.com.

Findings

Medium

• [scope-creep] internal/forge/forge.go — Five stub methods are added to the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

Low

• [missing-field] internal/forge/fake.go — CreateProtectedVariable records a VariableRecord with Protected defaulting to false, even though the method inherently creates a protected variable. By contrast, UpdateVariable correctly records the protected parameter. If test code inspects CreatedProtectedVars records, the Protected field will be misleading.
  Remediation: Set Protected: true in the VariableRecord appended by CreateProtectedVariable.

• [test-coverage-gap] internal/forge/fake_test.go — No test exists for FakeClient.CreateProtectedVariable. All other new FakeClient methods (IsProtectedBranch, CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable) have corresponding tests.
  Remediation: Add a test that calls CreateProtectedVariable and verifies the record in CreatedProtectedVars.

• [docs-staleness] docs/ADRs/0044-deprecate-per-org-installation-mode.md:336,377 — References "Keep ListOrgInstallations" without noting the move to GitHubExtensions. This ADR is Accepted, so only minor annotations are appropriate. File is not in the diff.
  Remediation: Add a parenthetical: "Keep ListOrgInstallations (now on GitHubExtensions, per-repo app discovery)".

Prior review resolution

All findings from prior reviews have been evaluated:

Prior finding	Severity	Status
stale-reference (harness.go error message)	Medium	✅ Fixed — now lists github.com, gitlab.com
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
docs-staleness (deprecate-per-org-install.md)	Low	✅ Fixed — references updated to note GitHubExtensions
docs-staleness (gitlab-implementation.md)	Low	✅ Fixed — reflects implemented changes
breaking-change-documentation	High	✅ Fixed — BREAKING CHANGE: trailer present
error-handling-convention (IsNotSupported)	Medium	✅ Fixed — IsNotSupported() helper added
error-handling-inconsistency (runUninstall)	Medium	✅ Fixed — ErrNotSupported handled with user message
behavioral-change (recoverCreatedApp)	Medium	✅ Fixed — StepInfo message added
test-adequacy (schedule round-trip)	Low	✅ Fixed — round-trip fidelity added
test-adequacy (UpdateVariable protected)	Low	✅ Fixed — Protected field in VariableRecord
documentation-consistency (godoc)	Low	✅ Fixed — godoc comments added
parameter-naming-consistency	Low	✅ Fixed — semantic parameter names retained

Previous run (18)

Review

Verdict: comment

Well-structured Phase 0 forge interface preparation. The breaking change is properly marked (! suffix + BREAKING CHANGE: trailer), all callers are updated with correct type-assertion fallbacks, tests cover new functionality thoroughly, and security analysis found no issues. All high and medium findings from the prior review at 3aaf7893eb have been addressed — the commit message now includes the required BREAKING CHANGE: trailer, IsNotSupported() helper follows the established sentinel pattern, runUninstall properly handles ErrNotSupported with a user-facing message, recoverCreatedApp logs a StepInfo instead of silently skipping, and FakeClient has round-trip fidelity for pipeline schedules and records the protected flag on variable updates.

Findings

Medium

• [stale-reference] internal/harness/harness.go:787 — Error message hardcodes (github.com) as the only supported forge, but this PR updates IsSupportedForge to accept gitlab.com. When ParseForgeURL fails for an unsupported host (e.g., bitbucket.org), the user sees "must be hosted on a supported forge (github.com)" — omitting gitlab.com. This file is not in the diff.
  Remediation: Update to "a supported forge (github.com, gitlab.com)" or derive the list dynamically.

• [scope-creep] internal/forge/forge.go — Five stub methods are added to the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

Low

• [docs-staleness] docs/plans/deprecate-per-org-install.md — Multiple references (lines 36, 644, 670, 686, 704, 756, 774) describe ListOrgInstallations as retained on the forge.Client interface. It has moved to GitHubExtensions. The method is still retained functionally; only its interface home changed.

• [docs-staleness] docs/problems/gitlab-implementation.md:421-422 — Discusses moving ListOrgInstallations and GetAppClientID to extension interfaces as future work, but this has been implemented in this PR.

Prior review resolution

All findings from the prior review at SHA 3aaf7893eb have been evaluated:

Prior finding	Severity	Status
breaking-change-documentation	High	✅ Fixed — BREAKING CHANGE: trailer added
error-handling-convention (missing IsNotSupported helper)	Medium	✅ Fixed — IsNotSupported() added with godoc
error-handling-inconsistency (runUninstall)	Medium	✅ Fixed — proper ErrNotSupported handling with user message
behavioral-change (recoverCreatedApp)	Medium	✅ Fixed — StepInfo message added
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
test-adequacy (CreatePipelineSchedule map)	Low	✅ Fixed — round-trip fidelity added
test-adequacy (UpdateVariable protected)	Low	✅ Fixed — Protected field added to VariableRecord
documentation-consistency (godoc)	Low	✅ Fixed — godoc comments added to PipelineSchedule and GitHubExtensions
parameter-naming-consistency	Low	✅ Fixed — semantic parameter names retained in stub methods

---

Labels: PR modifies forge abstraction layer (internal/forge/) and Go source exclusively

Previous run (19)

Review

Verdict: comment

Well-structured Phase 0 forge interface preparation. The breaking change is properly marked (! suffix + BREAKING CHANGE: trailer), all callers are updated with correct type-assertion fallbacks, tests cover new functionality thoroughly, and security analysis found no issues. All high and medium findings from the prior review at 3aaf7893eb have been addressed — the commit message now includes the required BREAKING CHANGE: trailer, IsNotSupported() helper follows the established sentinel pattern, runUninstall properly handles ErrNotSupported with a user-facing message, recoverCreatedApp logs a StepInfo instead of silently skipping, and FakeClient has round-trip fidelity for pipeline schedules and records the protected flag on variable updates.

Findings

Medium

• [stale-reference] internal/harness/harness.go:787 — Error message hardcodes (github.com) as the only supported forge, but this PR updates IsSupportedForge to accept gitlab.com. When ParseForgeURL fails for an unsupported host (e.g., bitbucket.org), the user sees "must be hosted on a supported forge (github.com)" — omitting gitlab.com. This file is not in the diff.
  Remediation: Update to "a supported forge (github.com, gitlab.com)" or derive the list dynamically.

• [scope-creep] internal/forge/forge.go — Five stub methods are added to the base forge.Client interface (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) with no in-tree callers yet, expanding the contract all forge implementations must satisfy. Severity anchored from prior review. This is explicitly authorized by ADR-0067 and the gitlab-cron-polling implementation plan as Phase 0 interface preparation.

Low

• [docs-staleness] docs/plans/deprecate-per-org-install.md — Multiple references (lines 36, 644, 670, 686, 704, 756, 774) describe ListOrgInstallations as retained on the forge.Client interface. It has moved to GitHubExtensions. The method is still retained functionally; only its interface home changed.

• [docs-staleness] docs/problems/gitlab-implementation.md:421-422 — Discusses moving ListOrgInstallations and GetAppClientID to extension interfaces as future work, but this has been implemented in this PR.

Prior review resolution

All findings from the prior review at SHA 3aaf7893eb have been evaluated:

Prior finding	Severity	Status
breaking-change-documentation	High	✅ Fixed — BREAKING CHANGE: trailer added
error-handling-convention (missing IsNotSupported helper)	Medium	✅ Fixed — IsNotSupported() added with godoc
error-handling-inconsistency (runUninstall)	Medium	✅ Fixed — proper ErrNotSupported handling with user message
behavioral-change (recoverCreatedApp)	Medium	✅ Fixed — StepInfo message added
scope-creep (5 stub methods)	Medium	🔄 Persists — authorized by ADR-0067
test-adequacy (CreatePipelineSchedule map)	Low	✅ Fixed — round-trip fidelity added
test-adequacy (UpdateVariable protected)	Low	✅ Fixed — Protected field added to VariableRecord
documentation-consistency (godoc)	Low	✅ Fixed — godoc comments added to PipelineSchedule and GitHubExtensions
parameter-naming-consistency	Low	✅ Fixed — semantic parameter names retained in stub methods

---

Labels: PR modifies forge abstraction layer (internal/forge/) and Go source exclusively

Previous run (20)

Review

Findings

High

• [breaking-change-documentation] — The commit message includes the ! suffix marking a breaking change, which is correct. However, it lacks the required BREAKING CHANGE: trailer. Per COMMITS.md: "Both the ! suffix and the trailer are required. The ! suffix signals the breaking change to human reviewers and enables future automated tooling; the trailer tells users what to do about it." The PR body documents the migration path, but the commit message trailer is required for GoReleaser release notes.
  Remediation: Add a BREAKING CHANGE: trailer to the commit message, e.g.: BREAKING CHANGE: ListOrgInstallations and GetAppClientID are removed from forge.Client. Callers must type-assert to forge.GitHubExtensions.

Medium

• [error-handling-convention] internal/forge/forge.go:66 — The new ErrNotSupported sentinel error lacks an IsNotSupported() helper function. All six existing sentinel errors in this file (ErrNotFound, ErrAlreadyExists, ErrBranchProtected, ErrForbidden, ErrNoChanges, ErrNonFastForward) follow the pattern of pairing each error with a corresponding Is{ErrorName} helper.
  Remediation: Add func IsNotSupported(err error) bool { return errors.Is(err, ErrNotSupported) }

• [error-handling-inconsistency] internal/cli/admin.go:1823 — In runUninstall, when the forge.Client does not implement GitHubExtensions, listErr is set to forge.ErrNotSupported. The subsequent code path assumes all apps are installed and opens browser URLs to github.com/organizations/... for uninstallation. On a non-GitHub forge, presenting GitHub-specific uninstall URLs is incorrect behavior — ErrNotSupported should be distinguished from transient communication failures.
  Remediation: When ErrNotSupported, skip the app uninstall section or inform the user that app uninstallation is not available on this forge.

• [scope-creep] internal/forge/forge.go — The PR adds five stub methods to forge.Client (CreatePipelineSchedule, DeletePipelineSchedule, ListPipelineSchedules, UpdateVariable, CreateProtectedVariable) that return ErrNotSupported on GitHub. While the implementation plan documents this as intentional Phase 0 preparation, adding these methods now expands the interface contract that all future forge implementations must satisfy.

• [behavioral-change] internal/appsetup/appsetup.go:349 — In recoverCreatedApp, when the forge.Client does not implement GitHubExtensions, the entire candidate recovery loop is silently skipped and (nil, nil) is returned. The behavior is logically correct (non-GitHub forges don't use the manifest flow), but a debug log line would improve diagnosability.

Low

• [missing-authorization] internal/forge/forge.go — No linked issue for this non-trivial refactor. The work is authorized by ADR-0067 and the gitlab-cron-polling-implementation.md plan, which provide stronger authorization than a GitHub issue.

• [architecture-coherence] internal/forge/forge.go — Type assertions in callers (appsetup, admin, CLI) may appear to conflict with ADR-0005's promise that forge additions require no caller changes. ADR-0067 explicitly addresses this: the changes are limited to calling new forge-neutral methods, not adding forge-conditional logic.

• [documentation-consistency] internal/forge/forge.go — The new PipelineSchedule struct and GitHubExtensions interface lack godoc comments. All other types and interfaces in this file have doc comments.

• [parameter-naming-consistency] internal/forge/github/github.go — Stub methods use _ for all parameters (e.g., _, _, _, _, _ string), but the established pattern in this file uses _ only for context.Context when unused. Other parameters retain semantic names.

• [test-coverage-gap] internal/forge/github/github_test.go — No tests verify callers' type assertion fallback paths (the negative case when GitHubExtensions is not available). This gap is acceptable for Phase 0 since no GitLab client exists yet.

• [naming-consistency] internal/forge/forge.go — GitHubExtensions is the first forge-specific interface name in the package. The design rationale is sound (these methods have no GitLab analogue), but it sets a naming precedent.

• [test-adequacy] internal/forge/fake.go:1609 — UpdateVariable accepts a protected bool parameter but the recorded VariableRecord does not include it, preventing test assertions on the flag.

• [test-adequacy] internal/forge/fake.go:1584 — CreatePipelineSchedule does not update the PipelineSchedules map, so there is no round-trip fidelity between creating and listing.

• [error-handling-inconsistency] internal/cli/github.go:904 — In runGitHubUninstall, ErrNotSupported is treated as a transient failure with a misleading error message. Low risk since this function is GitHub-specific by design.

• [fail-open-assessment] internal/appsetup/appsetup.go:266 — When GitHubExtensions type assertion fails, lookupErr is set to forge.ErrNotFound, skipping the public-app reuse check. This is fail-closed for security but means a non-GitHub forge would always attempt the manifest flow.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:31 AM UTC · Completed 11:45 AM UTC
Commit: 5eb7fbb · View workflow run →

Superseded by updated review

[fullsend-ai-review]

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

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:11 PM UTC · Completed 12:30 PM UTC
Commit: 723b228 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:36 PM UTC · Completed 12:47 PM UTC
Commit: 0dd5ad9 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:48 PM UTC · Completed 1:01 PM UTC
Commit: 5fa1dbf · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:09 PM UTC · Completed 1:20 PM UTC
Commit: 3c1558b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:25 PM UTC · Completed 1:36 PM UTC
Commit: 0c7da2f · View workflow run →

[waynesun09]

Approved — review-squad round 5 (fix verification + ADR review).

All three medium findings from round 4 are verified fixed at 0c7da2f3: the GitLab guard in parseAgentSourceURL (with rejection test), the appsetup non-GitHub test now reaching the type-assertion path, and the CI-variable rename (UpdateCIVariable/CreateProtectedCIVariable with name params and corrected godoc). internal/forge, internal/forge/github, and internal/appsetup test suites pass; the TestRunAgent_* failures in internal/cli are environmental and unrelated to this diff.

Outstanding, non-blocking:

1. PipelineSchedule.CronTimezone is unsettable through the Client API — see the existing thread. Dead surface with no callers yet; fine as a follow-up when the GitLab implementation lands.

2. ADR 0067 convention issues (per skills/writing-adrs), non-blocking but worth a follow-up PR:

   • Size: ~483 content lines vs the documented <80 target (>100 is the repo's own red-flag threshold). Most Decision subsections (credential model, cron poller mechanics, tier tables, security comparison) duplicate material that lives in the 1,916-line companion plan — they could compress to 1–2 sentences plus a link.
   • One decision per ADR: the credential model (bot PAT, WIF vs protected-variable modes, auto-selection) is an independent decision bundled into the event-dispatch ADR — architecture.md already cites it as answering the per-role identity question, which suggests it deserves its own ADR. The forge-abstraction section likewise restates this PR's interface changes and could shrink to a cross-reference.
   • Consequences format: 15 bullets + 5-item risk list + comparison table vs the 3–5 one-sentence-bullet target; the risk list and comparison table fit better in the plan or problem docs.
   • Minor: the forge-method list in the ADR omits ListPipelineSchedules and CreateProtectedCIVariable.

Convention checks that pass for ADR 0067: template compliance (frontmatter, status match, no leading zeros, valid relates_to), cross-references (ADR 0028 already deprecated with a forward link), and living-doc updates (architecture.md and the gitlab-implementation problem doc are already annotated).

[fullsend-ai-retro]

🤖 Retro · ❌ Terminated · Started 2:30 PM UTC · Ended 2:39 PM UTC
Commit: de2f978 · View workflow run →

[fullsend-ai-retro]

PR #3194 is a large Go interface refactoring (840+/86-, 27 files) to prepare the forge package for GitLab support. The automated review agent ran 12 times across 3 days but its initial pass only found procedural/medium-low issues. The human reviewer (waynesun09, operating as a review-squad with 4-8 LLM agents) found 10+ medium/high-severity correctness findings across 4 rounds that the automated review agent never independently discovered — most critically, a resource leak that 4/8 review-squad agents caught independently. Qodo also found 4 bugs in its first pass that the review agent missed. Three proposals target the correctness sub-agent's Go-specific analysis capabilities:

1. Resource lifecycle checklist — The agent analyzed IsProtectedBranch in detail across 12 runs but missed an unclosed http.Response.Body on the success path.
2. Interface signature cross-validation — The agent cited the implementation plan as authorization but never compared interface signatures against it.
3. Validation permissiveness consumer check — The agent's consumer-completeness check covers enum additions but missed that ParseForgeURL becoming more permissive required checking all 4 callers.

Proposals filed

• Correctness sub-agent should check Go resource lifecycle patterns (defer Close, http.Response.Body) (in fullsend-ai/fullsend)
• Correctness sub-agent should compare new interface signatures against referenced design documents (in fullsend-ai/fullsend)
• Correctness sub-agent consumer-completeness check should cover validation function permissiveness changes (in fullsend-ai/fullsend)

[ralphbean]

LGTM. One non-blocking note inline.

[ralphbean]

[non-blocking] ghExt here is only non-nil when ghOK is true, and this line is only reachable when listErr == nil, which requires ghOK. So it's safe today — but the invariant is implicit and spread across ~30 lines of branching. Could we guard this with if !ghOK { ... } or restructure so ghExt stays in scope only where it's valid? Mostly thinking about a future editor who changes the branching above and gets a nil deref.

[rh-hemartin]

This is marked as a breaking change, so we will be broken on our org?