Skip to content

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

Merged
rh-hemartin merged 1 commit into
mainfrom
agent/2294-sandbox-scoped-providers
Jun 18, 2026
Merged

fix(#2294): make EnsureProvider idempotent via update on AlreadyExists#2323
rh-hemartin merged 1 commit into
mainfrom
agent/2294-sandbox-scoped-providers

Conversation

@rh-hemartin

@rh-hemartin rh-hemartin commented Jun 16, 2026

Copy link
Copy Markdown
Member

Make EnsureProvider idempotent: when openshell provider create returns AlreadyExists, fall back to openshell provider update with the current credentials and config. This fixes repeated fullsend run invocations against the same gateway failing at the "Ensuring provider" step.

Adds buildProviderUpdateArgs helper and tests covering the update fallback and non-AlreadyExists error propagation.

Closes #2294

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown

Site preview

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

Commit: 58c0e940f98275e08ecb8f5d3ba5a28d5c4132c1

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:45 AM UTC · Completed 11:56 AM UTC
Commit: 182028a · View workflow run →

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [error-handling] internal/sandbox/sandbox.go:119 — The AlreadyExists detection relies on substring matching against unstructured CLI output ("provider already exists"). If openshell changes the error message, the fallback is benign (original error propagation). See also: [error-handling-duplication] finding at this location.
  • [error-handling-duplication] internal/sandbox/sandbox.go:140 — The secret-redaction loop (for _, s := range secrets { outStr = strings.ReplaceAll(outStr, s, "***") }) is duplicated in both EnsureProvider and updateProvider. A small redactSecrets(output string, secrets []string) string helper would eliminate the duplication.
  • [error-message-clarity] internal/sandbox/sandbox.go:144 — The update error message "provider update %q failed" does not mention the fallback context. A user seeing this in logs may not understand why an update was attempted. Consider: fmt.Errorf("provider update (after create conflict) %q failed: %s", name, outStr).

Info

  • [documentation-convention] internal/sandbox/sandbox.go:149buildProviderUpdateArgs comment is less detailed than buildProviderArgs, which explains the bare-key security rationale and links to external docs.
  • [stale-behavioral-description] docs/guides/dev/cli-internals.md:402 — CLI internals documentation describes EnsureProvider() as "Register model provider (bare-key form)". After this PR, the operation is idempotent (create-or-update) but the description does not reflect this.
  • [stale-behavioral-description] docs/ADRs/0030-openshell-sandbox-interaction-model.md:123 — ADR-0030 states the runner "calls openshell provider create" for each provider. After this PR, it may also call provider update. A minor annotation noting idempotent behavior would keep the ADR current.
  • [missing-new-identifier] docs/superpowers/specs/2026-05-06-openshell-native-sandbox-transport-design.md:128 — Design document lists EnsureProvider() — openshell provider create under functions calling openshell directly. After this PR, it may also call provider update.
Previous run

Review

Findings

Low

  • [error-handling] internal/sandbox/sandbox.go:119 — The AlreadyExists detection uses strings.Contains(outStr, "provider already exists") on combined stdout+stderr. While more specific than the prior "AlreadyExists" match, it still relies on substring matching against unstructured CLI output. If openshell changes the error message, the check silently falls back to the original error-propagation behavior (benign failure mode). See also: [string-matching-robustness] finding at this location.
  • [string-matching-robustness] internal/sandbox/sandbox.go:119 — The "provider already exists" check uses case-sensitive matching. The codebase's isAlreadyExistsError in internal/forge/github/github.go uses strings.ToLower before matching. Case-insensitive matching would be more robust and consistent with established patterns.
  • [error-handling-duplication] internal/sandbox/sandbox.go:119 — The error output redaction logic (convert to string, loop over secrets, replace with ***) is duplicated between EnsureProvider and updateProvider. The codebase generally extracts such repeated patterns into helper functions. Consider extracting a redactSecrets helper.
  • [test-adequacy] internal/sandbox/sandbox_test.go:516TestEnsureProvider_AlreadyExists_FallsBackToUpdate fake script exits 0 for any subcommand that is not create. The test would pass even if updateProvider invoked a wrong subcommand. The fake script should verify $2 = "update" and fail otherwise.
  • [design-decision-documentation] internal/sandbox/sandbox.go:119 — Issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 discusses three approaches for idempotency. The code comment explains the intent ("Provider exists from a prior run — update it with current credentials") but the choice of update-on-exists over silent success or delete+recreate could benefit from a brief rationale note.
  • [stale-behavioral-description] docs/guides/dev/cli-internals.md:402 — CLI internals documentation describes EnsureProvider() as "Register model provider (bare-key form)". This is accurate but incomplete — it does not mention that the operation is now idempotent (create-or-update).
  • [stale-behavioral-description] docs/ADRs/0030-openshell-sandbox-interaction-model.md:123 — ADR-0030 describes the credential delivery mechanism stating the runner "calls openshell provider create" for each provider. The create path is still primary; the update fallback is an implementation detail, but a minor annotation noting the idempotent behavior would keep the ADR current.

Info

  • [test-coverage] internal/sandbox/sandbox_test.go:509 — No test covers the case where create returns AlreadyExists and the subsequent update also fails. This is a valid runtime failure mode worth covering to verify error propagation and secret redaction in the update error path.
  • [issue-scope-alignment] internal/sandbox/sandbox.go — Issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 mentions a related idempotency gap in InferenceLayer.Install(). The PR correctly scopes to EnsureProvider only. Consider filing a follow-up issue for the InferenceLayer gap.
  • [documentation-convention] internal/sandbox/sandbox.go:142buildProviderUpdateArgs comment is less detailed than buildProviderArgs, which explains the bare-key security rationale and links to external docs. Consider expanding to match.
  • [missing-new-identifier] docs/superpowers/specs/2026-05-06-openshell-native-sandbox-transport-design.md:128 — Design document lists EnsureProvider() — openshell provider create under functions calling openshell directly. After this PR, it may also call provider update.
Previous run

Review

Findings

Medium

  • [error-handling] internal/sandbox/sandbox.go:119 — The AlreadyExists detection uses a bare substring match (strings.Contains(outStr, "AlreadyExists")) on the combined stdout+stderr of the failed openshell provider create command. If openshell changes its error format, or if a different error message incidentally contains "AlreadyExists", the code will incorrectly attempt an update instead of propagating the real error. The test at line 512 uses "status: AlreadyExists, message: provider already exists" — matching the more specific prefix would be safer.
    Remediation: Use a more specific match such as strings.Contains(outStr, "status: AlreadyExists") or match against a structured error code if openshell provides one.

Low

  • [credential-rotation-scope] internal/sandbox/sandbox.go:106 — The PR implements credential update semantics (overwriting existing provider credentials) as the idempotency mechanism. While the EnsureProvider docstring already says "creates or updates," issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 discusses three approaches and the choice of update-on-exists (option 3) warrants a brief note in the PR or issue explaining why update was chosen over silent success.
  • [test-adequacy] internal/sandbox/sandbox_test.go:516TestEnsureProvider_AlreadyExists_FallsBackToUpdate fake script exits 0 for any subcommand that is not create. The test would pass even if updateProvider invoked a wrong subcommand (e.g., provider delete). buildProviderUpdateArgs has its own unit test, but the integration test should verify the update subcommand was actually called.
  • [error-handling-consistency] internal/sandbox/sandbox.go:118 — The error output redaction logic (convert to string, loop over secrets, replace with ***) is duplicated between EnsureProvider and updateProvider. Consider extracting a redactSecrets helper.
  • [string-matching-robustness] internal/sandbox/sandbox.go:119 — The codebase's isAlreadyExistsError in internal/forge/github/github.go uses strings.ToLower before matching. The openshell CLI may always emit AlreadyExists as a PascalCase status code, but case-insensitive matching would be more robust.
  • [naming-consistency] internal/sandbox/sandbox.go:142buildProviderUpdateArgs uses a positional name while buildProviderArgs uses --name/--type flags. The PR comment explains this is intentional ("The update subcommand takes a positional name"), but documenting known differences between create and update CLI surfaces would help future maintainers.
  • [issue-scope-alignment] internal/sandbox/sandbox.go — Issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 mentions a related idempotency gap in InferenceLayer.Install(). The PR correctly scopes to EnsureProvider only, but consider filing a follow-up issue for the InferenceLayer gap.

Info

  • [test-coverage] internal/sandbox/sandbox_test.go:509 — No test covers the case where create returns AlreadyExists and the subsequent update also fails. This is a valid failure mode worth covering.
  • [test-naming-convention] internal/sandbox/sandbox_test.go:487FallsBackToUpdate implies degraded behavior, but this is the expected idempotent path. Consider UpdatesExistingProvider or CallsUpdate.
Previous run

Review

Findings

Low

  • [error-handling] internal/sandbox/sandbox.go:119 — The AlreadyExists detection uses strings.Contains(outStr, "provider already exists") on combined stdout+stderr. While more specific than the prior "AlreadyExists" match, it still relies on substring matching against unstructured CLI output. If openshell changes the error message, the check silently falls back to the original error-propagation behavior (benign failure mode). See also: [string-matching-robustness] finding at this location.
  • [string-matching-robustness] internal/sandbox/sandbox.go:119 — The "provider already exists" check uses case-sensitive matching. The codebase's isAlreadyExistsError in internal/forge/github/github.go uses strings.ToLower before matching. Case-insensitive matching would be more robust and consistent with established patterns.
  • [error-handling-duplication] internal/sandbox/sandbox.go:119 — The error output redaction logic (convert to string, loop over secrets, replace with ***) is duplicated between EnsureProvider and updateProvider. The codebase generally extracts such repeated patterns into helper functions. Consider extracting a redactSecrets helper.
  • [test-adequacy] internal/sandbox/sandbox_test.go:516TestEnsureProvider_AlreadyExists_FallsBackToUpdate fake script exits 0 for any subcommand that is not create. The test would pass even if updateProvider invoked a wrong subcommand. The fake script should verify $2 = "update" and fail otherwise.
  • [design-decision-documentation] internal/sandbox/sandbox.go:119 — Issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 discusses three approaches for idempotency. The code comment explains the intent ("Provider exists from a prior run — update it with current credentials") but the choice of update-on-exists over silent success or delete+recreate could benefit from a brief rationale note.
  • [stale-behavioral-description] docs/guides/dev/cli-internals.md:402 — CLI internals documentation describes EnsureProvider() as "Register model provider (bare-key form)". This is accurate but incomplete — it does not mention that the operation is now idempotent (create-or-update).
  • [stale-behavioral-description] docs/ADRs/0030-openshell-sandbox-interaction-model.md:123 — ADR-0030 describes the credential delivery mechanism stating the runner "calls openshell provider create" for each provider. The create path is still primary; the update fallback is an implementation detail, but a minor annotation noting the idempotent behavior would keep the ADR current.

Info

  • [test-coverage] internal/sandbox/sandbox_test.go:509 — No test covers the case where create returns AlreadyExists and the subsequent update also fails. This is a valid runtime failure mode worth covering to verify error propagation and secret redaction in the update error path.
  • [issue-scope-alignment] internal/sandbox/sandbox.go — Issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 mentions a related idempotency gap in InferenceLayer.Install(). The PR correctly scopes to EnsureProvider only. Consider filing a follow-up issue for the InferenceLayer gap.
  • [documentation-convention] internal/sandbox/sandbox.go:142buildProviderUpdateArgs comment is less detailed than buildProviderArgs, which explains the bare-key security rationale and links to external docs. Consider expanding to match.
  • [missing-new-identifier] docs/superpowers/specs/2026-05-06-openshell-native-sandbox-transport-design.md:128 — Design document lists EnsureProvider() — openshell provider create under functions calling openshell directly. After this PR, it may also call provider update.
Previous run (2)

Review

Findings

Medium

  • [error-handling] internal/sandbox/sandbox.go:119 — The AlreadyExists detection uses a bare substring match (strings.Contains(outStr, "AlreadyExists")) on the combined stdout+stderr of the failed openshell provider create command. If openshell changes its error format, or if a different error message incidentally contains "AlreadyExists", the code will incorrectly attempt an update instead of propagating the real error. The test at line 512 uses "status: AlreadyExists, message: provider already exists" — matching the more specific prefix would be safer.
    Remediation: Use a more specific match such as strings.Contains(outStr, "status: AlreadyExists") or match against a structured error code if openshell provides one.

Low

  • [credential-rotation-scope] internal/sandbox/sandbox.go:106 — The PR implements credential update semantics (overwriting existing provider credentials) as the idempotency mechanism. While the EnsureProvider docstring already says "creates or updates," issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 discusses three approaches and the choice of update-on-exists (option 3) warrants a brief note in the PR or issue explaining why update was chosen over silent success.
  • [test-adequacy] internal/sandbox/sandbox_test.go:516TestEnsureProvider_AlreadyExists_FallsBackToUpdate fake script exits 0 for any subcommand that is not create. The test would pass even if updateProvider invoked a wrong subcommand (e.g., provider delete). buildProviderUpdateArgs has its own unit test, but the integration test should verify the update subcommand was actually called.
  • [error-handling-consistency] internal/sandbox/sandbox.go:118 — The error output redaction logic (convert to string, loop over secrets, replace with ***) is duplicated between EnsureProvider and updateProvider. Consider extracting a redactSecrets helper.
  • [string-matching-robustness] internal/sandbox/sandbox.go:119 — The codebase's isAlreadyExistsError in internal/forge/github/github.go uses strings.ToLower before matching. The openshell CLI may always emit AlreadyExists as a PascalCase status code, but case-insensitive matching would be more robust.
  • [naming-consistency] internal/sandbox/sandbox.go:142buildProviderUpdateArgs uses a positional name while buildProviderArgs uses --name/--type flags. The PR comment explains this is intentional ("The update subcommand takes a positional name"), but documenting known differences between create and update CLI surfaces would help future maintainers.
  • [issue-scope-alignment] internal/sandbox/sandbox.go — Issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294 mentions a related idempotency gap in InferenceLayer.Install(). The PR correctly scopes to EnsureProvider only, but consider filing a follow-up issue for the InferenceLayer gap.

Info

  • [test-coverage] internal/sandbox/sandbox_test.go:509 — No test covers the case where create returns AlreadyExists and the subsequent update also fails. This is a valid failure mode worth covering.
  • [test-naming-convention] internal/sandbox/sandbox_test.go:487FallsBackToUpdate implies degraded behavior, but this is the expected idempotent path. Consider UpdatesExistingProvider or CallsUpdate.
Previous run (3)

Review

Findings

High

  • [logic-error] internal/cli/run.go:493sandboxProviderNames may be a strict subset of h.Providers, silently dropping providers from the sandbox. h.Providers is a list of logical names declared in the harness YAML. providerDefs is loaded from YAML files in the providers/ directory via LoadProviderDefs. If a harness declares a provider name in h.Providers that has no corresponding definition file, the original code would still pass that name to CreateWithRetry (as a --provider flag to openshell sandbox create). The PR replaces h.Providers with sandboxProviderNames, which is only populated from providerDefs. Any provider in h.Providers without a definition file is silently dropped from the CreateWithRetry call, potentially breaking sandbox creation or leaving the sandbox without access to that provider.
    Remediation: After building sandboxProviderNames from providerDefs, account for providers in h.Providers that were NOT in providerDefs. Either add them unchanged (they are pre-existing gateway providers) or fail explicitly with a clear error message.

Low

  • [scope-creep] internal/cli/run.go:456 — The PR implements sandbox-scoped provider lifecycle management which goes beyond the scope originally discussed in issue EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs #2294. The issue requested idempotent EnsureProvider behavior and offered three approaches. This PR implements a fourth approach not discussed in the issue. The design is coherent, but the decision should be documented in the issue.
  • [race-condition] internal/cli/run.go:448sandboxName uses time.Now().Unix() with 1-second granularity. This is a pre-existing limitation (not a regression), but extending the name to gateway-global providers means a collision would affect more resources. Consider using time.Now().UnixNano() or a random suffix.
  • [documentation-comment-style] internal/sandbox/sandbox.go:128 — Doc comments for DeleteProvider and SandboxProviderName could be slightly expanded to match the verbose pattern used by some other functions in this file, though the existing comments are adequate by Go standards.

Info

  • [missing-doc] docs/guides/dev/cli-internals.md:409DeleteProvider() is not documented in the 'Key Sandbox Operations' table.
  • [missing-doc] docs/guides/dev/cli-internals.md:280 — The 'Sandbox Lifecycle' diagram doesn't show the new provider scoping behavior or cleanup step.

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread internal/cli/run.go Outdated
Comment thread internal/cli/run.go Outdated
Comment thread internal/sandbox/sandbox.go Outdated

@maruiz93 maruiz93 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a nice idea to delete the providers at the end but it would be better to update them if they already exist while running fullsend run (between different iterations) related inline comments posted

Comment thread internal/cli/run.go
Comment thread internal/cli/run.go Outdated
Comment thread internal/sandbox/sandbox.go Outdated
Comment thread internal/cli/run.go Outdated
Comment thread internal/sandbox/sandbox_test.go Outdated
Comment thread internal/cli/run.go Outdated
@rh-hemartin
rh-hemartin force-pushed the agent/2294-sandbox-scoped-providers branch from 182028a to 88faa44 Compare June 17, 2026 08:09
@rh-hemartin rh-hemartin changed the title feat(provider): create sandbox-scoped providers and delete them after fix(#2294): make EnsureProvider idempotent via update on AlreadyExists Jun 17, 2026
@rh-hemartin
rh-hemartin requested a review from maruiz93 June 17, 2026 08:11
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:12 AM UTC · Completed 8:25 AM UTC
Commit: 88faa44 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 17, 2026
@rh-hemartin
rh-hemartin force-pushed the agent/2294-sandbox-scoped-providers branch from 88faa44 to 50d1e8a Compare June 17, 2026 08:28
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:34 AM UTC · Completed 8:55 AM UTC
Commit: 50d1e8a · View workflow run →

if err != nil {
// Redact known credential values from error output.
outStr := string(out)
// openshell emits: code: 'Some entity that we attempted to create already exists', message: "provider already exists"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] error-handling

The AlreadyExists detection uses strings.Contains(outStr, "provider already exists") on combined stdout+stderr. While more specific than the prior "AlreadyExists" match, it still relies on substring matching against unstructured CLI output. If openshell changes the error message, the check silently falls back to the original error-propagation behavior (benign failure mode).

Comment thread internal/sandbox/sandbox.go
Comment thread internal/sandbox/sandbox.go
Comment thread internal/sandbox/sandbox_test.go
Comment thread internal/sandbox/sandbox.go
Comment thread internal/sandbox/sandbox_test.go
Comment thread internal/sandbox/sandbox.go
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 17, 2026
When openshell provider create returns AlreadyExists, fall back to
openshell provider update so repeated fullsend run invocations against
the same gateway succeed without manual provider deletion.

Adds buildProviderUpdateArgs helper and tests covering the fallback
and non-AlreadyExists error propagation paths.

Refs #2294

Signed-off-by: Hector Martinez <hemartin@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 17, 2026

Copy link
Copy Markdown

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

Comment thread internal/sandbox/sandbox.go
Comment thread internal/sandbox/sandbox.go
Comment thread internal/sandbox/sandbox.go
Comment thread internal/sandbox/sandbox.go
@fullsend-ai-review fullsend-ai-review Bot removed the ready-for-merge All reviewers approved — ready to merge label Jun 17, 2026
@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 17, 2026
@rh-hemartin
rh-hemartin added this pull request to the merge queue Jun 18, 2026
Merged via the queue into main with commit fcd4101 Jun 18, 2026
16 checks passed
@rh-hemartin
rh-hemartin deleted the agent/2294-sandbox-scoped-providers branch June 18, 2026 06:05
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 18, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 6:10 AM UTC · Completed 6:23 AM UTC
Commit: 58c0e94 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2323 — make EnsureProvider idempotent

Workflow overview: Issue #2294 → triage → code agent PR #2296 (delete-and-recreate) → human rejected, created PR #2323 (sandbox-scoped providers) → human reviewer redirected to update-on-AlreadyExists → 3 force pushes → merged after 4 bot reviews and 2 human reviews.

What went well:

  • The review bot's initial CHANGES_REQUESTED on fix(#2294): make EnsureProvider idempotent via update on AlreadyExists #2323 caught a valid logic error in the sandbox-scoped approach.
  • After the approach pivot, the bot correctly approved with only low-severity findings — appropriate signal calibration.
  • Human reviewer (maruiz93) provided high-value design feedback that guided the PR to the right solution.
  • The final implementation (create-then-update-on-AlreadyExists) is clean and correct.

Inefficiencies observed:

No new proposals. All identified improvement opportunities are already covered by existing open issues, including proposals filed by the prior retro on the related agent PR #2296. This workflow is a good example of the human-agent collaboration model working correctly — the bot provided fast implementation-level feedback while humans provided the design-level guidance that shaped the final solution.

rh-hemartin added a commit that referenced this pull request Jun 18, 2026
Nothing reads this variable since the provider migration (#2323).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Hector Martinez <hemartin@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EnsureProvider should be idempotent — fails with AlreadyExists on repeated runs

2 participants