Skip to content

feat!: isolate provider configuration per run - #93

Merged
monatis merged 4 commits into
mainfrom
review/pr-90-run-scoped-provider-config
Jul 22, 2026
Merged

feat!: isolate provider configuration per run#93
monatis merged 4 commits into
mainfrom
review/pr-90-run-scoped-provider-config

Conversation

@efecnc

@efecnc efecnc commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Status

Replacement review PR for #90. The original PR was merged accidentally and cannot be reopened by GitHub. PR #91 removed its changes from main; this PR restores the change for review and must remain open until a human explicitly authorizes merge.

Review only: do not merge automatically.

Breaking 0.11 change

This PR deliberately versions the provider-isolation work as IsanAgent 0.11.0. It removes process-global and independently mutable provider configuration instead of adding unsafe compatibility behavior.

Removed 0.10 API 0.11 replacement
set_fallback_providers(specs) AgentLogic::new_with_fallback_providers(params, specs)
agent.provider_credentials_handle() plus independent writes Build the provider and call switch_provider_with_credentials(provider, credentials).await

switch_provider and set_provider_credentials remain temporary shims with documented limitations. New integrations should use the atomic provider-plus-credentials API.

What changed

  • remove the process-global fallback provider registry
  • atomically own the active provider and credentials per AgentLogic
  • snapshot provider, credential identity, and filtered fallbacks when each run is admitted
  • preserve that snapshot for queued inbounds and sub-agent runs
  • make model switches affect later admissions without mutating active work
  • add an explicit instance-scoped failover constructor
  • bump the crate to 0.11.0 and document the migration
  • add a compile-checked migration example

Risk review

The behavioral boundary is admission time: an active run retains its old provider; an inbound accepted after a model switch retains the new provider even while queued. Fallback keys stay instance-local and Debug output is redacted. Manual compaction has no run admission and intentionally reads the current instance provider.

Validation

  • cargo check --all-features
  • cargo fmt --all -- --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --doc --all-features (1 passed)
  • cargo test --all-features (445 passed, 0 failed, 6 ignored; plus 1 doctest)
  • git diff --check

Original PR: #90

@amazon-q-developer amazon-q-developer 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.

The run-scoped provider snapshot implementation is well-structured and addresses the concurrency concern correctly. The changes ensure provider/credential pairs are atomic and isolated per admitted run, preventing race conditions when switching models during concurrent execution. Documentation updates across AGENTS.md, README.md, and the API surface doc properly explain the new behavior and migration path for embedders.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

@efecnc

efecnc commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Codex review — blocking finding

Public API removals are not versioned as breaking changes.

This diff removes the public set_fallback_providers(...) function and AgentLogic::provider_credentials_handle() method, while Cargo.toml remains at 0.10.0. Existing embedders using either API will fail to compile, despite the PR/docs describing AgentLogic::new and the legacy switch methods as source-compatible migration paths. The new paired provider API is safer, but it does not make those symbol removals source-compatible.

Before merge, choose one explicit contract:

  1. preserve deprecated compatibility shims with safe, documented behavior; or
  2. bump the crate to the next breaking pre-1.0 version (for example 0.11.0) and add a migration note that names both removed APIs and their replacements.

A public-surface/compile regression test would prevent this from being missed again. All runtime isolation tests, full tests, fmt, and clippy -D warnings otherwise pass.

Review status: keep open; not ready to merge until the API/versioning decision is resolved.

@efecnc

efecnc commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Recommended resolution

I recommend treating this as an intentional 0.11.0 breaking release, rather than adding compatibility shims.

A shim for set_fallback_providers would reintroduce process-global mutable fallback state, and a mutable provider_credentials_handle() shim would reopen the provider/credential split-brain race that this PR is designed to remove. Preserving those APIs would therefore weaken the core guarantee of the change.

Concrete changes before merge

  1. Bump the crate version from 0.10.0 to 0.11.0 and update the lockfile.
  2. Mark the PR/title as breaking (feat!) and add a short 0.11 migration section to the public API documentation/changelog.
  3. Document these exact replacements:
    • set_fallback_providers(specs) → pass specs to AgentLogic::new_with_fallback_providers(params, specs).
    • provider_credentials_handle() or separate provider/credential writes → call switch_provider_with_credentials(provider, credentials).await.
    • Existing switch_provider(...) / set_provider_credentials(...) callers may compile temporarily, but their compatibility-shim behavior and limitations must remain explicit.
  4. Add one compileable documentation example covering construction plus an atomic runtime switch. A larger compatibility layer is unnecessary.
  5. Keep the downstream ALTAI repin/API migration in a separate PR after this PR is approved; do not merge that downstream change ahead of the upstream contract decision.

This keeps the implementation small, makes the break honest, and preserves the run-isolation invariant without carrying unsafe legacy global state.

@efecnc efecnc changed the title feat: isolate provider configuration per run feat!: isolate provider configuration per run Jul 22, 2026
@efecnc

efecnc commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Implemented the requested release-contract changes in 605297a:

  • bumped the crate from 0.10.0 to 0.11.0
  • marked the PR as a breaking change
  • documented exact replacements for the removed public APIs
  • documented the remaining compatibility shims and their limitations
  • added a compile-checked migration example
  • re-ran check, fmt, clippy with warnings denied, doctests, the full all-features test suite, and diff checks successfully

The versioning/documentation blocker is resolved from my review standpoint. This PR remains open and unmerged for human review; no merge action will be taken.

@monatis monatis 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.

Merging this PR, but we need to remove the legacy two-stage provider switching to prevent any misuse. altai-app should exclusively use switch_provider_with_credentials, and isanagent should be free from legacy artifacts after migration.

@monatis
monatis merged commit 76c264e into main Jul 22, 2026
1 check passed
@monatis
monatis deleted the review/pr-90-run-scoped-provider-config branch July 23, 2026 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants