feat(forge): add ListRepoVariables, DeleteRepoVariable, DeleteRepoSecret

[ggallen]

Summary

• Adds ListRepoVariables, DeleteRepoVariable, DeleteRepoSecret to the forge.Client interface
• GitHub implementation with paginated list and idempotent deletes
• FakeClient implementation with deletion tracking for test assertions
• Foundation for fullsend repos status/sync/remove commands (ADR 0057, PR 3 of 8)

Test plan

• httptest-based tests for all three GitHub methods
• Pagination, empty results, idempotent deletes, error handling
• Coverage >85% on new code
• go build ./... passes — all interface implementations compile

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

feat(forge): add repo variable listing and secret/variable deletes

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Extend forge.Client with list-variables and delete-secret/variable operations.
• Implement GitHub REST support with pagination and idempotent secret deletes.
• Enhance FakeClient + add httptest coverage for new GitHub behaviors.

Diagram

graph TD
  Cmd["fullsend repos *"] --> IFace["forge.Client"] --> Live["github.LiveClient"] --> GH{{"GitHub REST API"}}
  Tests["github_test.go"] --> IFace
  Tests --> Fake["forge.FakeClient"]
  IFace --> Fake

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use Link-header pagination (instead of total_count + page loop)

• ➕ Aligns with GitHub's standard pagination mechanism
• ➕ Avoids relying on total_count semantics and reduces off-by-one risks
• ➕ Naturally terminates without an arbitrary max-page cap
• ➖ Requires parsing Link headers and slightly more plumbing in the HTTP layer
• ➖ Harder to unit test unless the test harness exposes headers explicitly

2. Return a slice of variable structs instead of map[string]string

• ➕ Preserves ordering and allows future metadata (e.g., created_at) without signature changes
• ➕ Avoids silent overwrite if duplicate names ever appear
• ➖ Less ergonomic for consumers that need direct lookup by name
• ➖ Would require more refactors in upcoming commands expecting a map

3. Adopt an upstream GitHub client library for Actions variables/secrets

• ➕ Outsourced pagination and request/response typing
• ➕ Potentially less custom error-handling code
• ➖ New dependency surface and versioning considerations
• ➖ May not match the repo's existing HTTP conventions and APIError handling

Recommendation: The PR’s approach (small interface expansion + direct REST implementation + httptest coverage) is a good fit for the existing forge abstraction and keeps dependencies minimal. Consider switching pagination to Link-header following (or at least documenting/guarding the 100-page cap) if very large repos are expected, but the current total_count-driven loop is reasonable for typical variable counts.

Files changed (4) +233 / -0

Enhancement (2) +53 / -0

forge.goExtend forge.Client interface with ListRepoVariables and DeleteRepoSecret +2/-0

Extend forge.Client interface with ListRepoVariables and DeleteRepoSecret

• Adds three repo-level methods to the Client interface: DeleteRepoSecret, ListRepoVariables, and (already present) DeleteRepoVariable remains part of the contract. This ensures LiveClient and FakeClient compile against a unified surface for upcoming repos commands.

internal/forge/forge.go

github.goImplement GitHub ListRepoVariables (paginated) and idempotent DeleteRepoSecret +51/-0

Implement GitHub ListRepoVariables (paginated) and idempotent DeleteRepoSecret

• Implements ListRepoVariables by fetching Actions variables pages (per_page=100) until total_count is satisfied or an empty page is returned. Adds DeleteRepoSecret using DELETE /actions/secrets/{name} and treats 204 and 404 as success for idempotency.

internal/forge/github/github.go

Tests (1) +134 / -0

github_test.goAdd httptest coverage for variable listing pagination and secret deletes +134/-0

Add httptest coverage for variable listing pagination and secret deletes

• Introduces tests for ListRepoVariables across single-page, paginated, empty, and API-error scenarios. Adds DeleteRepoSecret tests covering success, idempotent 404 handling, and unexpected status errors.

internal/forge/github/github_test.go

Other (1) +46 / -0

fake.goTrack and implement repo secret/variable deletion + variable listing in FakeClient +46/-0

Track and implement repo secret/variable deletion + variable listing in FakeClient

• Adds FakeClient support for DeleteRepoSecret and ListRepoVariables and augments DeleteRepoVariable to record deletions. Introduces DeletedSecrets/DeletedVariables tracking to enable test assertions and removes stored values when present.

internal/forge/fake.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:58 PM UTC · Completed 12:10 AM UTC
Commit: a48a239 · View workflow run →

[github-actions]

Site preview

Preview: https://1539ad03-site.fullsend-ai.workers.dev

Commit: f1021e5fa275f87e27b0d400b86599782e89f898

[codecov]

Codecov Report

❌ Patch coverage is 94.28571% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/forge/github/github.go	87.50%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 54 rules

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

1. Fake list misses created vars ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

FakeClient.ListRepoVariables only enumerates VariableValues, but
FakeClient.CreateOrUpdateRepoVariable does not populate VariableValues/VariablesExist, so a
create-then-list flow returns an empty result. This makes the fake diverge from the LiveClient
behavior and can break tests that rely on the fake for variable workflows.

Code

internal/forge/fake.go[R856-872]

+func (f *FakeClient) ListRepoVariables(_ context.Context, owner, repo string) (map[string]string, error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	if e := f.err("ListRepoVariables"); e != nil {
+		return nil, e
+	}
+
+	prefix := owner + "/" + repo + "/"
+	result := make(map[string]string)
+	for key, val := range f.VariableValues {
+		if strings.HasPrefix(key, prefix) {
+			name := strings.TrimPrefix(key, prefix)
+			result[name] = val
+		}
+	}
+	return result, nil

Relevance

⭐⭐⭐ High

Team often fixes FakeClient/live semantic drift; consistency changes accepted in FakeClient (e.g.,
PR #1215, #689).

PR-#1215
PR-#689
PR-#2201

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

CreateOrUpdateRepoVariable records variables in f.Variables only, while the newly added
ListRepoVariables builds its result exclusively by scanning f.VariableValues, so created
variables won’t be listed unless tests manually populate VariableValues.

internal/forge/fake.go[809-873]

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.ListRepoVariables` reads from `f.VariableValues`, but `FakeClient.CreateOrUpdateRepoVariable` only records into `f.Variables` and does not update `VariableValues` (or `VariablesExist`). As a result, creating/updating a variable via the fake does not affect what `ListRepoVariables` returns.

### Issue Context
This PR introduces `ListRepoVariables` and upcoming features are likely to use create/list/delete flows in tests. The fake should model basic state transitions similarly to the live implementation.

### Fix Focus Areas
- internal/forge/fake.go[809-824]
- internal/forge/fake.go[856-873]

Suggested implementation direction:
- In `CreateOrUpdateRepoVariable`, set:
 - `f.VariableValues[owner+"/"+repo+"/"+name] = value` (initializing the map if nil)
 - `f.VariablesExist[...] = true` (initializing if nil)
- Keep appending to `f.Variables` for call recording.
- Optionally, ensure `ListRepoVariables` safely handles `VariableValues == nil` (it already ranges safely), but the key fix is to keep the fake’s write/read backing stores consistent.

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

---

2. Silent pagination truncation ✓ Resolved 🐞 Bug ≡ Correctness

Description

LiveClient.ListRepoVariables stops after 100 pages and still returns success, even if more variables
may remain, which can yield incomplete results without any error. This contradicts the method
comment that it follows pagination until all variables are fetched.

Code

internal/forge/github/github.go[R1843-1868]

+	for page := 1; page <= 100; page++ {
+		path := fmt.Sprintf("/repos/%s/%s/actions/variables?per_page=100&page=%d", owner, repo, page)
+		resp, err := c.get(ctx, path)
+		if err != nil {
+			return nil, fmt.Errorf("list repo variables page %d: %w", page, err)
+		}
+
+		var body struct {
+			TotalCount int `json:"total_count"`
+			Variables  []struct {
+				Name  string `json:"name"`
+				Value string `json:"value"`
+			} `json:"variables"`
+		}
+		if err := decodeJSON(resp, &body); err != nil {
+			return nil, fmt.Errorf("decode repo variables page %d: %w", page, err)
+		}
+
+		for _, v := range body.Variables {
+			result[v.Name] = v.Value
+		}
+
+		if len(result) >= body.TotalCount || len(body.Variables) == 0 {
+			break
+		}
+	}

Relevance

⭐⭐ Medium

Repo uses 100-page caps elsewhere; reviewers favored documenting API limits (partially accepted in
PR #1040).

PR-#1040
PR-#816

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation uses for page := 1; page <= 100; page++ with no error path for exceeding that
cap, and the doc comment states pagination is followed until all variables are fetched.

internal/forge/github/github.go[1837-1871]

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.ListRepoVariables` hard-caps pagination at 100 pages but does not signal an error if that cap is reached while additional pages may still exist. This can produce a partial name→value map while returning `nil` error.

### Issue Context
The function’s docstring says it follows pagination until all variables are fetched, but the implementation can exit the `for page := 1; page <= 100; page++` loop due to the page cap and still return `result, nil`.

### Fix Focus Areas
- internal/forge/github/github.go[1837-1871]

Suggested implementation direction:
- Keep a max-pages guard for safety, but if `page == maxPages` and termination conditions are not met (e.g., `len(body.Variables) > 0` and `len(result) < body.TotalCount`), return an explicit error indicating pagination exceeded the cap.
- Alternatively, switch the stop condition to the standard `len(body.Variables) < perPage` and still error if max pages is reached while `len(body.Variables) == perPage` (indicating there may be more).

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

---

[fullsend-ai-review]

Looks good to me.

Clean addition of ListRepoVariables, DeleteRepoSecret, and deletion tracking for DeleteRepoVariable to the forge abstraction layer. Implementations follow established patterns (mutex-guarded FakeClient, paginated GitHub API calls with idempotent deletes), and tests cover the key scenarios (single page, pagination, empty results, API errors, idempotent 404 handling).

Low

• [edge-case] internal/forge/github/github.go — In ListRepoVariables, the pagination termination uses a fetched counter independent of the result map size. If the GitHub API returns duplicate variable names across pages (due to concurrent variable creation/deletion causing pagination drift), fetched would reach totalCount even though the map contains fewer unique entries. This is benign — the len(body.Variables) == 0 guard and page <= 100 hard cap prevent infinite looping.

• [error-injection] internal/forge/fake_test.go — TestFakeClient_ErrorInjection table test does not include the new DeleteRepoSecret and ListRepoVariables methods. Each method has its own dedicated error injection sub-test, so coverage exists, but the centralized table test has a consistency gap.

• [thread-safety] internal/forge/fake_test.go — TestFakeClient_ThreadSafety does not exercise the new DeleteRepoSecret and ListRepoVariables methods under the race detector. While both methods use mutex locking correctly, adding them to the centralized thread-safety test would provide stronger verification.

Previous run

Looks good to me.

Clean addition of ListRepoVariables, DeleteRepoSecret, and deletion tracking for DeleteRepoVariable to the forge abstraction layer. Implementations follow established patterns (mutex-guarded FakeClient, paginated GitHub API calls with idempotent deletes), and tests cover the key scenarios (single page, pagination, empty results, API errors, idempotent 404 handling).

Low

• [edge-case] internal/forge/github/github.go — In ListRepoVariables, the pagination termination condition len(result) >= body.TotalCount uses the accumulated map size rather than a running count of received items. If the API returns duplicate variable names across pages (due to concurrent variable creation/deletion causing pagination drift), the map would be smaller than expected, potentially causing an extra page fetch. This is benign — the len(body.Variables) == 0 guard and page <= 100 hard cap prevent infinite looping.

---

Labels: PR adds Go code to the forge interface and GitHub implementation.

Previous run

Looks good to me.

Clean addition of ListRepoVariables, DeleteRepoSecret, and deletion tracking for DeleteRepoVariable to the forge abstraction layer. Implementations follow established patterns (mutex-guarded FakeClient, paginated GitHub API calls with idempotent deletes), and tests cover the key scenarios (single page, pagination, empty results, API errors, idempotent 404 handling).

Low

• [edge-case] internal/forge/github/github.go — In ListRepoVariables, the pagination termination condition len(result) >= totalCount uses the accumulated map size rather than a running count of received items. If the API returns duplicate variable names across pages (due to concurrent variable creation/deletion causing pagination drift), the map would be smaller than expected, potentially causing an extra page fetch. This is benign — the len(body.Variables) == 0 guard and page <= 100 hard cap prevent infinite looping.

• [missing-interface-method] internal/forge/forge.go — The implementation plan (repos-management.md) mentions adding ListOrgReposIncludePrivate in the same section as these methods. Verify whether it is deferred to a later PR in the 8-PR sequence.

Previous run (2)

Looks good to me.

Clean addition of ListRepoVariables, DeleteRepoSecret, and deletion tracking for DeleteRepoVariable to the forge abstraction layer. Implementations follow established patterns (mutex-guarded FakeClient, paginated GitHub API calls with idempotent deletes), and tests cover the key scenarios (single page, pagination, empty results, API errors, idempotent 404 handling).

Low

• [edge-case] internal/forge/github/github.go:1864 — In ListRepoVariables, the pagination termination condition len(result) >= body.TotalCount uses the accumulated map size rather than a running count of received items. If the API returns duplicate variable names across pages (due to concurrent variable creation/deletion causing pagination drift), the map would be smaller than expected, potentially causing an extra page fetch. This is benign — the len(body.Variables) == 0 guard and page <= 100 hard cap prevent infinite looping.

---

Labels: PR adds Go code to the forge interface and GitHub implementation.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:22 AM UTC · Completed 12:33 AM UTC
Commit: 2f00987 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:41 AM UTC · Completed 12:53 AM UTC
Commit: f1021e5 · View workflow run →