fix(#2294): make EnsureProvider idempotent via delete-and-recreate by fullsend-ai-coder[bot] · Pull Request #2296 · fullsend-ai/fullsend

fullsend-ai-coder · 2026-06-15T10:39:54Z

EnsureProvider called openshell provider create and treated any error as a hard failure. When a provider already existed from a prior run, the AlreadyExists error blocked subsequent runs, requiring manual cleanup between iterations.

Now when the create command fails with AlreadyExists, the function deletes the existing provider and recreates it with current credentials. This makes the function truly idempotent (matching the "Ensure" naming convention used by EnsureGateway and the Provider.Provision() interface contract) while also ensuring credentials are never stale across runs.

Also extracted a redactSecrets helper to reduce duplication in error formatting paths.

Closes #2294

Post-script verification

Branch is not main/master (agent/2294-idempotent-ensure-provider)
Secret scan passed (gitleaks — 9ca6edc3770e16b0f37a6e79046cfba7d01bffc9..HEAD)
Pre-commit hooks passed (authoritative run on runner)
Tests ran inside sandbox

EnsureProvider called `openshell provider create` and treated any error as a hard failure. When a provider already existed from a prior run, the AlreadyExists error blocked subsequent runs, requiring manual cleanup between iterations. Now when the create command fails with AlreadyExists, the function deletes the existing provider and recreates it with current credentials. This makes the function truly idempotent (matching the "Ensure" naming convention used by EnsureGateway and the Provider.Provision() interface contract) while also ensuring credentials are never stale across runs. Also extracted a redactSecrets helper to reduce duplication in error formatting paths. Closes #2294

github-actions · 2026-06-15T10:40:08Z

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

github-actions · 2026-06-15T10:41:30Z

Site preview

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

Commit: fac85e457735eec3a3fe4b23edc38b1e6a2639e1

codecov · 2026-06-15T10:43:32Z

Codecov Report

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

Files with missing lines	Patch %	Lines
internal/sandbox/sandbox.go	85.71%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-06-15T10:43:35Z

🤖 Finished Review · ✅ Success · Started 10:43 AM UTC · Completed 10:55 AM UTC
Commit: fac85e4 · View workflow run →

fullsend-ai-review · 2026-06-15T10:55:30Z

Review

Findings

Medium

[architectural-coherence] internal/sandbox/sandbox.go:103 — The function docstring says "creates or updates a provider" but the implementation is delete-and-recreate, which is neither create-if-absent nor update-in-place. The godoc should be updated to accurately reflect the actual semantics (e.g., "creates a provider, deleting and recreating it if it already exists to ensure credentials are current").

Low

[missing-test] internal/sandbox/sandbox_test.go — No test covers the path where delete succeeds but the retry create fails. This is the most dangerous error path (provider deleted but not recreated), and a test verifying the error message contains "failed after delete" would be valuable.
[edge-case] internal/sandbox/sandbox.go:121 — The AlreadyExists detection uses strings.Contains against combined stdout+stderr. If openshell changes its error format, the detection silently breaks and falls through to the generic error path. The fallback is safe (returns an error), but loses idempotency. Consider whether openshell provides a structured exit code that could be used instead.
[intent-scope-alignment] internal/sandbox/sandbox.go:117 — Issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 proposed three approaches. Option 2 (delete-and-recreate, chosen here) also refreshes credentials on every conflict, which is a useful property but goes slightly beyond pure idempotency. This is a reasonable design choice given the issue's analysis of credential staleness.
[architectural-coherence] internal/sandbox/sandbox.go:117 — Delete-and-recreate introduces a timing window where provider deletion could affect concurrent sandbox operations if the gateway is shared. In practice this is unlikely given the per-run lifecycle, but worth documenting if concurrent usage becomes a concern.

Info

[race-condition] internal/sandbox/sandbox.go:123 — Theoretical TOCTOU window between delete and retry create. Non-actionable given single-process usage; same pattern exists in CreateWithRetry.
[architectural-trajectory] internal/sandbox/sandbox.go:117 — Delete-and-recreate is a different reconciliation model from WIF provider create-or-update elsewhere. Not necessarily wrong, but creates divergent patterns worth being aware of.

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review · 2026-06-15T10:55:31Z

+		// If the provider already exists, delete it and recreate with
+		// current credentials. This keeps EnsureProvider idempotent and
+		// ensures credentials are never stale across runs.
+		if strings.Contains(string(out), "AlreadyExists") {


[low] edge-case

AlreadyExists detection uses strings.Contains against combined stdout+stderr. If openshell changes its error format, the detection silently breaks and falls through to the generic error path — safe fallback, but loses idempotency.

fullsend-ai-review · 2026-06-15T10:55:31Z

@@ -115,16 +115,34 @@ func EnsureProvider(name, providerType string, credentials, config map[string]st
 	cmd.Env = append(os.Environ(), extraEnv...)
 	out, err := cmd.CombinedOutput()
 	if err != nil {


[low] intent-scope-alignment

Issue #2294 proposed three approaches. Option 2 (delete-and-recreate) also refreshes credentials on every conflict, which goes slightly beyond pure idempotency but is a reasonable design choice.

fullsend-ai-review · 2026-06-15T10:55:31Z

@@ -115,16 +115,34 @@ func EnsureProvider(name, providerType string, credentials, config map[string]st
 	cmd.Env = append(os.Environ(), extraEnv...)
 	out, err := cmd.CombinedOutput()
 	if err != nil {


[low] architectural-coherence

Delete-and-recreate introduces a timing window where provider deletion could affect concurrent sandbox operations if the gateway is shared.

fullsend-ai-review · 2026-06-15T10:55:31Z

+		// ensures credentials are never stale across runs.
+		if strings.Contains(string(out), "AlreadyExists") {
+			delCmd := exec.Command("openshell", "provider", "delete", name)
+			if delOut, delErr := delCmd.CombinedOutput(); delErr != nil {


[info] race-condition

Theoretical TOCTOU window between delete and retry create. Non-actionable given single-process usage; same pattern exists in CreateWithRetry.

fullsend-ai-review · 2026-06-15T10:55:32Z

@@ -115,16 +115,34 @@ func EnsureProvider(name, providerType string, credentials, config map[string]st
 	cmd.Env = append(os.Environ(), extraEnv...)
 	out, err := cmd.CombinedOutput()
 	if err != nil {


[info] architectural-trajectory

Delete-and-recreate is a different reconciliation model from WIF provider create-or-update elsewhere. Creates divergent patterns worth noting.

rh-hemartin · 2026-06-16T11:41:58Z

Closed in favor of #2323

fullsend-ai-retro · 2026-06-16T11:46:53Z

🤖 Finished Retro · ✅ Success · Started 11:46 AM UTC · Completed 11:58 AM UTC
Commit: fac85e4 · View workflow run →

fullsend-ai-retro · 2026-06-16T11:58:56Z

Retro: PR #2296 — Agent chose reactive fix, human found preventive design

Outcome: PR #2296 (agent-authored) was closed without merging, superseded by human-authored PR #2323.

Timeline:

Issue #2294 filed describing EnsureProvider failing with AlreadyExists on repeated runs. Listed three reactive approaches (succeed silently, delete-and-recreate, hybrid).
Triage agent recommended catching AlreadyExists or delete-and-recreate — aligned with issue's options.
Code agent chose delete-and-recreate, produced PR #2296 in ~10 minutes.
Review agent flagged concurrency/TOCTOU risks as low/info severity, gave COMMENTED verdict (no approval), PR got requires-manual-review label.
Human (rh-hemartin) investigated deeper, realized the problem could be prevented by design using sandbox-scoped unique provider names — no collision possible, plus proper cleanup on teardown.
PR fix(#2294): make EnsureProvider idempotent via delete-and-recreate #2296 closed in favor of PR #2323.

What worked well:

The pipeline correctly flagged the PR for manual review (COMMENTED, not APPROVED)
Review agent identified the real architectural concerns (concurrency, TOCTOU, divergent pattern) — just rated them too low
End-to-end latency from issue to PR was fast (~20 minutes)

Filtered proposals:

Review agent strategic fitness dimension: Already covered by open issue #849. The review agent's low/info ratings on the concurrency findings would likely be escalated with a strategic fitness check.
Code agent approach selection: Partially covered by #2185 and #1891, though from different angles.

Proposals filed

Triage agent should consider preventive alternatives to reactive error handling (in fullsend-ai/fullsend)
Review agent should escalate architectural-coherence findings when they indicate a design-level alternative exists (in fullsend-ai/fullsend)

github-actions Bot deployed to site-preview June 15, 2026 10:41 View deployment

fullsend-ai-review Bot reviewed Jun 15, 2026

View reviewed changes

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 15, 2026

This was referenced Jun 15, 2026

fix(#2294): make EnsureProvider idempotent via delete-and-recreate guyoron1/fullsend#10

Closed

fix(#2294): make EnsureProvider idempotent via delete-and-recreate guyoron1/fullsend#15

Closed

rh-hemartin mentioned this pull request Jun 16, 2026

fix(#2294): make EnsureProvider idempotent via update on AlreadyExists #2323

Merged

rh-hemartin closed this Jun 16, 2026

rh-hemartin deleted the agent/2294-idempotent-ensure-provider branch June 16, 2026 11:42

fullsend-ai-retro Bot mentioned this pull request Jun 16, 2026

Triage agent should consider preventive alternatives to reactive error handling #2324

Open

fullsend-ai-retro Bot mentioned this pull request Jun 16, 2026

Review agent should escalate architectural-coherence findings when they indicate a design-level alternative exists #2325

Open

guyoron1 mentioned this pull request Jun 16, 2026

feat(provider): create sandbox-scoped providers and delete them after guyoron1/fullsend#20

Closed

Uh oh!

Conversation

fullsend-ai-coder Bot commented Jun 15, 2026

Post-script verification

Uh oh!

github-actions Bot commented Jun 15, 2026

E2E tests did not run

Uh oh!

github-actions Bot commented Jun 15, 2026

Site preview

Uh oh!

codecov Bot commented Jun 15, 2026

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jun 15, 2026

Review

Findings

Medium

Low

Info

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

rh-hemartin commented Jun 16, 2026

Uh oh!

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

Uh oh!

Uh oh!

fullsend-ai-retro Bot commented Jun 16, 2026

Retro: PR #2296 — Agent chose reactive fix, human found preventive design

Proposals filed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Jun 16, 2026 •

edited

Loading