chore(e2e): delete per-repo var before starting by rh-hemartin · Pull Request #2978 · fullsend-ai/fullsend

rh-hemartin · 2026-07-03T07:45:35Z

Summary

Introduces code to remove the flag on the test-repo on e2e tests to allow repo-maintenance to enroll it.

Related Issue

Fixes #2871

Changes

Clear FULLSEND_PER_REPO_INSTALL variable from test-repo before enrollment phase

Testing

make lint passes (stage changes first, then run)
Tests added/updated for new or modified logic

Checklist

PR title follows Conventional Commits (correct type, ! for breaking changes)
Commits are signed off (DCO) — human and human-directed agent sessions only
I wrote this contribution myself and can explain all changes in it

qodo-code-review · 2026-07-03T07:46:20Z

PR Summary by Qodo

E2E: clear per-repo Actions variable before admin install/uninstall test

🧪 Tests 🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Delete the per-repo Actions variable that can cause e2e enrollment to be skipped.
• Make the admin e2e test more repeatable across reruns by clearing stale state.
• Improve enrollment PR polling logs to include org/repo context.

Diagram

graph TD
  T["e2e/admin_test.go"] --> GH["gh CLI"] --> API{{"GitHub REST API"}} --> VAR[("Repo Actions variable")]
  REC["reconcile-repos.sh"] --> VAR
  subgraph Legend
    direction LR
    _test["Test code"] ~~~ _svc["CLI/Service"] ~~~ _ext{{"External API"}} ~~~ _db[("Stored state")]
  end

High-Level Assessment

The following are alternative approaches to this PR:

1. Call GitHub REST API directly from Go (net/http)

➕ Removes dependency on local gh binary behavior/version
➕ Easier to unit-test request/response handling
➕ More explicit error handling (status codes vs CLI output parsing)
➖ More code in the test suite (auth headers, URL construction, retries)
➖ May diverge from other e2e patterns if the repo standardizes on gh

2. Use GitHub SDK/client already used elsewhere in repo (if present)

➕ Consistent API usage patterns and auth handling
➕ Potentially better typed errors than CLI output strings
➖ Adds/extends a dependency surface in tests if not already used
➖ May require additional setup for enterprise/base URL variants

Recommendation: The current approach (best-effort deletion via gh api with a non-fatal warning on failure) is appropriate for e2e preflight cleanup and keeps the change minimal. If gh CLI availability/versioning becomes a recurring source of flakes, switch to a direct HTTP call so the test can key off status codes (404 vs auth/permission errors) without parsing CLI output.

Files changed (1) +16 / -2

Tests (1) +16 / -2

admin_test.goPreflight delete FULLSEND_PER_REPO_INSTALL repo variable before e2e run +16/-2

Preflight delete FULLSEND_PER_REPO_INSTALL repo variable before e2e run

• Adds a new “Phase 0” that attempts to delete the FULLSEND_PER_REPO_INSTALL Actions repository variable via 'gh api' before the install/enrollment phases run. Also improves enrollment PR polling logs and assertion messages to include org/repo context, aiding debugging when the PR is not visible yet.

e2e/admin/admin_test.go

fullsend-ai-review · 2026-07-03T07:46:36Z

🤖 Finished Review · ✅ Success · Started 7:46 AM UTC · Completed 7:55 AM UTC
Commit: 17a9341 · View workflow run →

github-actions · 2026-07-03T07:48:31Z

Site preview

Preview: https://cd0ed06c-site.fullsend-ai.workers.dev

Commit: d47d559d9da50ca5be018abc2fbccf3d026ed8d3

qodo-code-review · 2026-07-03T07:49:13Z

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. ~~No timeout for gh api~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new gh api subprocess is started with exec.Command and CombinedOutput() without any
context/timeout, so it can hang the entire e2e test run if gh stalls. This is inconsistent with
the existing e2e token resolution path, which already uses exec.CommandContext with a timeout to
avoid hangs.

Code

e2e/admin/admin_test.go[R90-94]

+	deleteVarCmd := exec.Command("gh", "api",
+		fmt.Sprintf("repos/%s/%s/actions/variables/FULLSEND_PER_REPO_INSTALL", env.org, testRepo),
+		"-X", "DELETE")
+	deleteVarCmd.Env = append(os.Environ(), "GITHUB_TOKEN="+env.token)
+	if deleteOut, deleteErr := deleteVarCmd.CombinedOutput(); deleteErr != nil {

Relevance

⭐⭐⭐ High
Team previously accepted adding exec.CommandContext timeout for gh in e2e to prevent hangs (PR2277).
PR-#2277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code invokes gh without a timeout, while existing e2e auth code already uses
exec.CommandContext with a 30s timeout to avoid indefinite blocking. This is a known
pattern/requirement in this codebase area (historically called out in PR review).

e2e/admin/admin_test.go[87-95]
e2e/admin/auth.go[17-29]
PR-#2277

Agent prompt

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

### Issue description
`TestAdminInstallUninstall` runs `gh api ... -X DELETE` using `exec.Command` and waits with `CombinedOutput()` without any context/timeout, allowing indefinite hangs.

### Issue Context
The e2e suite already treats hanging `gh` subprocesses as a real risk (e.g., `resolveLocalToken` wraps `gh auth token` with a 30s timeout).

### Fix
- Use `context.WithTimeout` (e.g., 30s) and `exec.CommandContext` for the `gh api` delete.
- If the context deadline is exceeded, fail fast with a clear message including captured output.

### Fix Focus Areas
- e2e/admin/admin_test.go[83-99]
- e2e/admin/auth.go[17-36]

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

2. ~~Guard deletion not enforced~~ ✗ Dismissed 🐞 Bug ☼ Reliability

Description

TestAdminInstallUninstall continues after a failed delete of FULLSEND_PER_REPO_INSTALL (except
for a brittle output substring check), without verifying the variable is actually gone. If the guard
remains set, repo-maintenance will skip enrollment for test-repo, and mergeEnrollmentPR will
fail later when the expected enrollment PR never appears.

Code

e2e/admin/admin_test.go[R87-98]

+	t.Log("=== Phase 0: Remove per-repo variable ===")
+	// Clear any stale per-repo guard variable from previous test runs.
+	// reconcile-repos.sh skips repos with FULLSEND_PER_REPO_INSTALL=true.
+	deleteVarCmd := exec.Command("gh", "api",
+		fmt.Sprintf("repos/%s/%s/actions/variables/FULLSEND_PER_REPO_INSTALL", env.org, testRepo),
+		"-X", "DELETE")
+	deleteVarCmd.Env = append(os.Environ(), "GITHUB_TOKEN="+env.token)
+	if deleteOut, deleteErr := deleteVarCmd.CombinedOutput(); deleteErr != nil {
+		if !strings.Contains(string(deleteOut), "Not Found") {
+			t.Logf("Warning: could not delete per-repo guard variable: %v\n%s", deleteErr, deleteOut)
+		}
+	}

Relevance

⭐⭐ Medium
Mixed precedent: team accepted guard fail-closed logic (PR967) but often downgrades cleanup failures
to warnings (PR1215).
PR-#967
PR-#1215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new Phase 0 delete step logs and continues on most delete failures, but the enrollment
reconciliation logic explicitly skips repos when FULLSEND_PER_REPO_INSTALL is true and also
skips (fail-closed) on non-404 API errors. The e2e test later hard-requires the enrollment PR to
exist; if enrollment is skipped due to the guard not being removed, the PR will not appear and the
test fails later and less diagnostically.

e2e/admin/admin_test.go[83-98]
internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh[164-196]
e2e/admin/admin_test.go[256-286]

Agent prompt

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

### Issue description
`TestAdminInstallUninstall` tries to delete the per-repo enrollment guard variable via `gh api ... -X DELETE`, but it only logs a warning for most failures and does not verify that the variable is actually absent before proceeding.

### Issue Context
`reconcile-repos.sh` skips enrollment when the guard variable value is `"true"`, and it also fails closed (skips) on non-404 API errors. Proceeding after a failed deletion can therefore prevent the enrollment PR from being created, causing a slower, downstream failure in `mergeEnrollmentPR`.

### Fix
- Add a bounded timeout to the delete operation.
- Treat non-404 delete failures as test failures (use `require.NoError` / `t.Fatalf`) OR immediately verify postcondition:
 - Check whether `FULLSEND_PER_REPO_INSTALL` still exists and equals `"true"` (via `env.client.GetRepoVariable`), and fail if it does.
 - If you keep using `gh api`, classify 404 robustly (e.g., by using `gh api -i` and checking status code, or by parsing the JSON `status` field like `reconcile-repos.sh` does).

### Fix Focus Areas
- e2e/admin/admin_test.go[83-99]
- internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh[164-196]
- e2e/admin/admin_test.go[256-286]

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

3. exec.Command("gh") in e2e test ✓ Resolved 📘 Rule violation ⌂ Architecture

Description

The PR adds a direct exec.Command("gh", ...) invocation outside internal/forge/github/, which
violates the repo policy restricting GitHub CLI usage to that package. This can bypass the standard
forge abstraction layer and increases risk of inconsistent auth/error-handling patterns.

Code

e2e/admin/admin_test.go[R90-97]

+	deleteVarCmd := exec.Command("gh", "api",
+		fmt.Sprintf("repos/%s/%s/actions/variables/FULLSEND_PER_REPO_INSTALL", env.org, testRepo),
+		"-X", "DELETE")
+	deleteVarCmd.Env = append(os.Environ(), "GITHUB_TOKEN="+env.token)
+	if deleteOut, deleteErr := deleteVarCmd.CombinedOutput(); deleteErr != nil {
+		if !strings.Contains(string(deleteOut), "Not Found") {
+			t.Logf("Warning: could not delete per-repo guard variable: %v\n%s", deleteErr, deleteOut)
+		}

Relevance

⭐ Low
Similar request to forbid gh exec outside internal/forge was rejected in PR2277.
PR-#2277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 1062053 restricts exec.Command("gh"...) usage to files under
internal/forge/github/. The changed code introduces exec.Command("gh", "api", ...) in
e2e/admin/admin_test.go, which is outside the allowed directory.
Rule 1062053: Restrict gh CLI exec.Command usage to internal/forge/github
e2e/admin/admin_test.go[90-97]

Agent prompt

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

## Issue description
`e2e/admin/admin_test.go` shells out to the GitHub CLI via `exec.Command("gh", ...)`. Per policy, `gh` CLI execution must be restricted to `internal/forge/github/`.

## Issue Context
The test currently deletes the repo Actions variable `FULLSEND_PER_REPO_INSTALL` by calling `gh api ... -X DELETE`. This should be done via the existing forge/GitHub client abstraction (or a new helper added under `internal/forge/github/` that the test calls), rather than invoking `gh` directly in the test.

## Fix Focus Areas
- e2e/admin/admin_test.go[90-97]

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

codecov · 2026-07-03T07:49:54Z

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/forge/github/github.go	75.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-07-03T07:55:08Z

Review

Findings

Low

[test-adequacy] internal/forge/fake_test.go:629 — TestFakeClient_ThreadSafety does not call DeleteRepoVariable from concurrent goroutines. All other FakeClient methods that acquire the mutex are exercised in this test to verify correctness under the race detector, but the new DeleteRepoVariable method is missing.
Remediation: Add _ = fc.DeleteRepoVariable(ctx, "o", "r", "n") to the goroutine body in TestFakeClient_ThreadSafety, alongside the existing DeleteOrgVariable call.

Labels: PR modifies e2e test infrastructure and forge interface

Previous run

Review

Findings

Low

[test-adequacy] internal/forge/fake_test.go:510 — The TestFakeClient_ErrorInjection table-driven test does not include DeleteRepoVariable. Every other method on FakeClient that checks f.err(...) has a corresponding entry in this table, leaving the error-injection path in FakeClient.DeleteRepoVariable untested.
[test-adequacy] internal/forge/fake_test.go:629 — The TestFakeClient_ThreadSafety test does not call DeleteRepoVariable from concurrent goroutines. All other FakeClient methods are exercised here to verify correctness under the race detector. The new method uses f.mu.Lock() correctly, but omitting it from this test leaves a gap in race-detection coverage.

Labels: PR modifies e2e test infrastructure and forge interface

Previous run (2)

Review

Reason: stale-head

The review agent reviewed commit bf3ba1b34ebbd512b0e9764be9c35215dc740180 but the PR HEAD is now be233f4c1f6813b2817e1367a1223e4733cc2016. This review was discarded to avoid approving unreviewed code.

Previous run (3)

Review

Findings

Medium

[error-handling-idiom] e2e/admin/admin_test.go:87 — The new Phase 0 variable deletion uses exec.Command("gh", "api", ...) to call the GitHub API. AGENTS.md explicitly prohibits exec.Command("gh", ...) outside internal/forge/github/. The existing cleanup.go helpers (deleteBranch, deleteShimWorkflow, closePR) all use http.DefaultClient.Do() with raw HTTP for similar GitHub API operations. While auth.go does use exec.Command("gh", "auth", "token"), that is a local credential-retrieval operation, not an API call.
Remediation: Use http.DefaultClient.Do() with http.NewRequestWithContext for the DELETE call, following the pattern in cleanup.go's deleteBranch() or closePR(). Alternatively, add a DeleteRepoVariable method to forge.Client.

Low

[organization] e2e/admin/admin_test.go:87 — Phase 0 cleanup logic is placed inline in TestAdminInstallUninstall, but similar cleanup operations (deleting branches, closing PRs, removing files) are extracted to helpers in cleanup.go. Consider extracting this to a helper function in cleanup.go to match the established pattern.
[design-coherence] e2e/admin/admin_test.go:86 — Phase 0 cleanup runs at test start, but the FULLSEND_PER_REPO_INSTALL variable is never set by this test — it leaks from external processes (reconcile-repos.sh). The cleanup is reasonable as defense-in-depth, but consider extending cleanupStaleResources to also remove this variable, providing both start-of-test and teardown coverage.

fullsend-ai-review · 2026-07-03T08:02:44Z

🤖 Review · ⚠️ Cancelled · Started 8:02 AM UTC · Ended 8:07 AM UTC
Commit: 0a95cac · View workflow run →

fullsend-ai-review · 2026-07-03T08:08:13Z

🤖 Finished Review · ✅ Success · Started 8:08 AM UTC · Completed 8:15 AM UTC
Commit: bf3ba1b · View workflow run →

fullsend-ai-review · 2026-07-03T08:15:16Z

/fs-review

fullsend-ai-review · 2026-07-03T08:16:04Z

🤖 Finished Review · ❌ Failure · Started 8:16 AM UTC · Completed 8:26 AM UTC
Commit: be233f4 · View workflow run →

ifireball

Lgtm

…urces Add DeleteRepoVariable to forge.Client interface. Implement for GitHub (REST API DELETE) and fake client. Call in cleanupStaleResources to delete stale per-repo guard variable from test-repo before each run. Idempotent — 404 treated as success. Prevents test flake when halfsend org has leftover per-repo guard from prior run. Fixes #2871 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>

fullsend-ai-review · 2026-07-03T08:35:27Z

🤖 Finished Review · ❌ Failure · Started 8:35 AM UTC · Completed 8:53 AM UTC
Commit: d47d559 · View workflow run →

fullsend-ai-retro · 2026-07-03T08:59:30Z

🤖 Finished Retro · ✅ Success · Started 8:59 AM UTC · Completed 9:07 AM UTC
Commit: d47d559 · View workflow run →

fullsend-ai-retro · 2026-07-03T09:07:44Z

PR #2978 added DeleteRepoVariable to the forge.Client interface to fix flaky e2e tests (issue #2871). The review agent performed well: it correctly identified a forge abstraction violation and two organizational improvements, all adopted by the author. Human review was minimal ("Lgtm"). However, the workflow was costly with 5 review runs (2 failed with 422 errors). The 422 and stale-head-redispatch issues are covered by existing issues (#2569, #1067, #2399). Two new proposals filed: one for a testing gap pattern, one tracking autonomy readiness for forge interface changes.

Proposals filed

Require thread safety test coverage when adding new forge.Client methods (in fullsend-ai/fullsend)
Track autonomy readiness: review agent fully drove improvements on forge interface PR (in fullsend-ai/fullsend)

rh-hemartin requested a review from a team as a code owner July 3, 2026 07:45

github-actions Bot deployed to site-preview July 3, 2026 07:48 View deployment

qodo-code-review Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread e2e/admin/admin_test.go Outdated

Comment thread e2e/admin/admin_test.go Outdated

rh-hemartin self-assigned this Jul 3, 2026

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jul 3, 2026

rh-hemartin force-pushed the fix/e2e-per-repo branch from 17a9341 to 7e747be Compare July 3, 2026 08:01

github-actions Bot deployed to site-preview July 3, 2026 08:04 View deployment

rh-hemartin force-pushed the fix/e2e-per-repo branch from 7e747be to bf3ba1b Compare July 3, 2026 08:07

github-actions Bot deployed to site-preview July 3, 2026 08:09 View deployment

rh-hemartin force-pushed the fix/e2e-per-repo branch from bf3ba1b to be233f4 Compare July 3, 2026 08:14

github-actions Bot deployed to site-preview July 3, 2026 08:17 View deployment

ifireball approved these changes Jul 3, 2026

View reviewed changes

rh-hemartin force-pushed the fix/e2e-per-repo branch from be233f4 to d47d559 Compare July 3, 2026 08:34

github-actions Bot deployed to site-preview July 3, 2026 08:36 View deployment

rh-hemartin added this pull request to the merge queue Jul 3, 2026

Merged via the queue into main with commit 0cb75a1 Jul 3, 2026
14 checks passed

rh-hemartin deleted the fix/e2e-per-repo branch July 3, 2026 08:57

fullsend-ai-retro Bot mentioned this pull request Jul 3, 2026

Require thread safety test coverage when adding new forge.Client methods #2983

Open

fullsend-ai-retro Bot mentioned this pull request Jul 3, 2026

Track autonomy readiness: review agent fully drove improvements on forge interface PR #2984

Open

Uh oh!

Conversation

rh-hemartin commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issue

Changes

Testing

Checklist

Uh oh!

qodo-code-review Bot commented Jul 3, 2026

PR Summary by Qodo

Uh oh!

fullsend-ai-review Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

qodo-code-review Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fullsend-ai-review Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Low

Review

Findings

Low

Review

Review

Findings

Medium

Low

Uh oh!

This comment was marked as outdated.

Uh oh!

fullsend-ai-review Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 3, 2026

Uh oh!

fullsend-ai-review Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ifireball left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fullsend-ai-retro Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-retro Bot commented Jul 3, 2026

Proposals filed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rh-hemartin commented Jul 3, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 3, 2026 •

edited

Loading

github-actions Bot commented Jul 3, 2026 •

edited

Loading

qodo-code-review Bot commented Jul 3, 2026 •

edited

Loading

codecov Bot commented Jul 3, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 3, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 3, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 3, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 3, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 3, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Jul 3, 2026 •

edited

Loading