fix(#6125): delete leaked repo-level FULLSEND_MINT_URL in e2e cleanup

[ggallen]

Summary

• Add FULLSEND_MINT_URL deletion to CleanupStaleResources so already-poisoned halfsend orgs self-heal at the start of the next admin E2E run
• Root cause: cfmint behaviour tests (ci(#5346): add CF Worker preview mint to BT driver #6037) write a repo-level FULLSEND_MINT_URL on test-repo that shadows the org-level variable and points to a torn-down CF Worker preview, breaking the OIDC mint step in dispatch.yml
• PR ci(e2e): delete leaked repo-level FULLSEND_MINT_URL from test-repo #6112 prevents future poisoning but does not heal existing state; this completes the fix

Closes #6125

Test plan

• Merge queue E2E TestAdminInstallUninstall passes (the cleanup runs at test start, deleting the leaked variable before install)
• Verify no regression on behaviour tests (this only touches admin E2E cleanup path)

Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Delete leaked FULLSEND_MINT_URL repo variable during admin E2E cleanup

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Delete repo-level FULLSEND_MINT_URL during admin E2E pre-test cleanup
• Self-heal orgs poisoned by prior cfmint behavior tests writing a stale repo variable
• Prevent stale CF Worker preview URL from breaking OIDC mint/dispatch in workflow runs

Diagram

graph TD
  A["Admin E2E run"] --> B["CleanupStaleResources"] --> C{{"Forge/GitHub API"}} --> D["Delete repo var: per-repo guard"] --> E["Delete repo var: FULLSEND_MINT_URL"] --> F["Delete stale branches"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. One-off remediation script/runbook

• ➕ Avoids adding more logic to the E2E cleanup path
• ➕ Can be executed immediately without waiting for E2E to run
• ➖ Does not guarantee future self-healing if poisoning reappears
• ➖ Requires manual intervention and operational coordination

2. Fix at source only (behavior tests always clean up)

• ➕ Keeps cleanup narrowly scoped to admin E2E concerns
• ➕ Prevents future state poisoning at the producer
• ➖ Does not repair already-poisoned orgs/repositories
• ➖ Relies on all producers always cleaning up correctly

3. Make dispatch/mint ignore repo-level FULLSEND_MINT_URL

• ➕ Reduces fragility from GitHub variable shadowing
• ➕ Hardens workflow against unexpected repo config
• ➖ Bigger behavioral change with higher regression risk
• ➖ May hide real configuration mistakes by silently ignoring repo overrides

Recommendation: Keep this PR’s approach: deleting the leaked repo-level FULLSEND_MINT_URL in CleanupStaleResources is the smallest change that reliably self-heals previously poisoned orgs and directly addresses the failure mode (repo variables shadow org variables). The source-side fix (PR #6112) is still valuable, but cleanup-based remediation is the most reliable backstop for existing state.

Files changed (1) +5 / -0

Bug fix (1) +5 / -0

cleanup.goDelete leaked FULLSEND_MINT_URL repo variable during admin E2E cleanup +5/-0

Delete leaked FULLSEND_MINT_URL repo variable during admin E2E cleanup

• Adds a pre-test cleanup step to delete the repo-level FULLSEND_MINT_URL variable in the test repo. This prevents a stale, behavior-test-created value from shadowing the org-level variable and breaking the workflow dispatch/mint step.

pkg/e2etest/cleanup.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:09 PM UTC · Completed 11:20 PM UTC

Commit: 689a596 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

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

1. Duplicate variable deletion 🐞 Bug ⚙ Maintainability

Description

CleanupStaleResources now deletes FULLSEND_MINT_URL via deleteRepoVariable and later deletes the
same variable again via client.DeleteRepoVariable, adding redundant API calls and two deletion code
paths to keep consistent.

Code

pkg/e2etest/cleanup.go[R82-83]

+	// value pointing at a torn-down CF Worker preview breaks dispatch.
+	deleteRepoVariable(ctx, token, org, TestRepo, "FULLSEND_MINT_URL", t)

Relevance

●●● Strong

Team has accepted changes reducing redundant/avoidable GitHub API work in e2e code; duplicate delete
should be removed.

PR-#1612

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The newly added REST deletion targets FULLSEND_MINT_URL on TestRepo, but the same function later
still deletes FULLSEND_MINT_URL via the forge client. Since the forge GitHub client deletion is
already idempotent on 404/204, the added call is redundant and increases maintenance surface area.

pkg/e2etest/cleanup.go[77-84]
pkg/e2etest/cleanup.go[112-120]
internal/forge/github/github.go[2233-2246]

Agent prompt

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

## Issue description
`CleanupStaleResources` deletes `FULLSEND_MINT_URL` twice: once via the direct GitHub REST helper (`deleteRepoVariable`) and later via `forge.Client.DeleteRepoVariable`. This duplicates behavior, adds an extra API call, and creates two paths to maintain.

## Issue Context
The `forge` GitHub client’s `DeleteRepoVariable` is already idempotent (treats 404 as success), so only one deletion mechanism is needed.

## Fix Focus Areas
- pkg/e2etest/cleanup.go[77-120]
- internal/forge/github/github.go[2233-2246]

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

---

Context

✅ Compliance rules (platform): 54 rules

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

Tip of the day

💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[fullsend-ai-review]

Review

Findings

Low

• [code-duplication] pkg/e2etest/cleanup.go:82 — The new deleteRepoVariable call duplicates step 8 (lines 107–115), which already deletes FULLSEND_MINT_URL via client.DeleteRepoVariable. Both are idempotent, so functionally harmless — this mirrors the pre-existing PerRepoGuardVar duplication (line 78 vs step 7). Consider consolidating both pairs in a future cleanup.

---

Labels: PR modifies e2e test cleanup code in pkg/e2etest/

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:30 AM UTC · Completed 12:45 AM UTC

Commit: 689a596 · View workflow run →

[fullsend-ai-retro]

Retro: PR #6126 — delete leaked repo-level FULLSEND_MINT_URL in e2e cleanup

Timeline

1. 21:53 UTC — ggallen filed issue #6125 describing that CleanupStaleResources needed a deleteRepoVariable call for FULLSEND_MINT_URL to self-heal already-poisoned orgs.
2. 21:54–21:58 — Triage agent ran successfully (~4.5 min). Correctly labeled type/bug, component/e2e, priority/high, ready-to-code and provided good analysis with a proposed test case.
3. 21:59–22:02 — Code agent run 1 concluded the fix was "already present" at lines 107–115 of cleanup.go (from commit 1230d669 in PR ci(e2e): delete leaked repo-level FULLSEND_MINT_URL from test-repo #6112). No branch created, no PR. Cost: ~$0.76.
4. 22:04 — ggallen commented /fs-code make sure to create a branch for this PR!
5. 22:05–22:08 — Code agent run 2 reached the same "already fixed" conclusion. No branch, no PR. Cost: ~$0.88.
6. 22:12 — ggallen commented with explicit line-by-line instructions: add deleteRepoVariable(ctx, token, org, TestRepo, "FULLSEND_MINT_URL", t) after line 78.
7. 22:13–22:16 — Code agent run 3 still concluded the fix was already present, despite receiving explicit instructions. No branch, no PR. Cost: ~$0.86.
8. 23:08 — ggallen gave up on the code agent and manually opened PR #6126 with the 5-line fix.
9. 23:08–23:21 — Review agent ran and approved. Correctly identified a low-severity code-duplication finding (the new deleteRepoVariable call duplicates a later client.DeleteRepoVariable at lines 107–115) and noted it mirrors a pre-existing pattern with PerRepoGuardVar. Applied ready-for-merge and component/e2e labels.
10. 23:59 — PR added to merge queue. First attempt failed at 00:06 due to a flaky e2e test — a GitHub API timeout in an unrelated behaviour scenario (fork-dispatch.feature:7, blob upload context deadline exceeded). Re-added at 00:15, merged successfully at 00:27.

What went well

• Triage agent performed correctly: fast, accurate labels, useful analysis.
• Review agent performed correctly: caught the only real finding (code duplication), accurately assessed severity as low, recognized the pre-existing pattern, and approved appropriately.
• Review agent autonomy: The human merged based solely on the agent's approval — no additional human review was needed. This is a positive signal for autonomy on focused e2e test changes.

What went wrong

• Code agent failed 3 times ($2.50 total cost, ~50 min wall-clock from first code run to human giving up). The agent found a functionally similar client.DeleteRepoVariable call for FULLSEND_MINT_URL at lines 107–115 and concluded the issue was already fixed. It did not understand that the issue was asking for an additional early cleanup (after line 78) using the deleteRepoVariable helper, matching the established pattern of PerRepoGuardVar being deleted both early (line 78) and late (lines 101–106).
• Explicit human instructions were ignored. On the third attempt, ggallen gave the agent exact code to add and exactly where to add it. The code-implementation skill's Step 7 unconditionally exits on "already fixed" with no mechanism to honor explicit human override.

Evidence for existing issues (not filing new proposals)

• fullsend#1820 ("Code agent: accept HUMAN_INSTRUCTION from /fs-code comment body"): This retro provides a concrete case where the lack of structured HUMAN_INSTRUCTION support cost 3 wasted agent runs and forced manual implementation. The human's explicit instructions in /fs-code comments were available only as unstructured issue comments, with no special treatment by the agent.
• agents#677 ("Code agent needs a structured way to say 'needs human input' instead of silently no-oping"): While feat: agent self-shutdown via kill switch on suspicious activity #677 explicitly excludes "already fixed" paths from scope, this retro shows the "already fixed" no-op can cause the same user frustration — the agent silently no-ops 3 times with no escalation path.
• fullsend#6080 is a different failure mode (Jira ticket number false match in the pre-script layer, not the in-sandbox Step 7 check that failed here).

Proposals filed

• Code-implementation skill Step 7 should not override explicit human /fs-code instructions (in fullsend-ai/agents)