Skip to content

feat(provider): reuse-or-separate credential flow for multi-instance providers - #281

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
feat/multi-instance-credential-reuse
Aug 27, 2026
Merged

feat(provider): reuse-or-separate credential flow for multi-instance providers#281
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
feat/multi-instance-credential-reuse

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

What this fixes

Today, provider add (and the manage/init dashboard "add" paths) treat a second instance of the same provider type as a hard credential collision: it always forces a distinct env var name, with no way to say "this new instance should authenticate with the same key as the existing one." There's no way to add, e.g., a second Anthropic instance pinned to a different model but sharing the same ANTHROPIC_API_KEY — the wizard renames it into a new var every time, splitting one account's credential across two names for no reason.

The strict collision check shipped in #225 (_prompt_env_var_collision / _resolve_env_var_overrides in amplifier_app_cli/commands/provider.py, with supporting logic in provider_config_utils.py).

What this does

When adding another instance of a provider type whose default credential env var is already claimed, the wizard now offers a choice instead of forcing a rename:

  1. Reuse <EXISTING_VAR> (default) — binds the new instance to the same ${ENV_VAR}. The secret field is never prompted for or overwritten; only the placeholder is persisted, so both instances resolve the same stored credential at runtime.
  2. Separate credential — unchanged from today's flow: derive/validate a distinct env var name, then either populate it through the existing secure-storage flow (KeyManager.save_key), or leave it blank to persist the placeholder UNSET for runtime injection (shell / CI / DTU passthrough).

Editing an existing instance recovers its binding from the stored placeholder (including "shared" when another configured instance still references the same var), so the reuse invariant survives edits, not just the initial add. Non-interactive mode carries the same semantics — an explicit binding (shared or separate) is exempt from the existing collision fail-loud guard, since it's intentional.

Runtime hardening (new, beyond the wizard): a provider instance's required secret placeholder that resolves to unset now fails loudly before session mount, instead of silently expanding to "" and letting the provider module fall back to its own ambient env var — which would otherwise route a "separate" instance through a different account's key. Optional/keyless secret fields (e.g. a local Chat Completions server) are unaffected.

No settings schema, amplifier-core, or provider-module contract changes — this is app-CLI wizard/runtime policy only.

Testing

New/updated coverage in tests/test_provider_instance_credentials.py (TestReuseVsSeparateCredentialBinding) and a new tests/test_runtime_credential_validation.py, covering all four spec paths plus non-interactive parity and the runtime hardening:

  • Shared: reuse choice binds to the existing var, never prompts for/overwrites the secret, is the default choice.
  • Separate: derives a distinct var; secure-storage save still works when a value is entered.
  • Unset-for-runtime-injection: a blank separate binding persists the placeholder unset instead of erroring.
  • Edit-preservation: editing either of two instances sharing a var recovers binding_modes={"shared": ...} so the edit never re-prompts for/overwrites the common secret.
  • Runtime fallback hardening: an unresolved required credential placeholder fails loud before mount (unit + integration test through resolve_bundle_config()); optional/keyless fields, shared bindings with a set var, and missing provider metadata are all confirmed not to raise.

Full suite: 1469 passed, 1 skipped, 13 deselected, 1 xfailed. Ruff/pyright clean (no new lint findings beyond pre-existing baseline warnings, confirmed by diffing against main before these changes).

This design was validated across 10 independent reference implementations produced in an internal first-turn evaluation of this exact issue, and follows the most consistent design among them (a named CredentialDecision/binding-mode contract), additionally folding in the runtime unset-fallback hardening one of those implementations added after adversarial review.

Conflict note

Draft PR #234 (full-screen interactive TUI runtime) also touches amplifier_app_cli/commands/provider.py and amplifier_app_cli/provider_config_utils.py. Since #234 is a draft, I haven't coordinated a rebase — expect a merge conflict when #234 is updated to target main after this lands, in both files' provider-add/edit code paths.

🤖 Generated with Amplifier

…providers

When adding a second instance of the same provider type, the wizard's
env-var-collision check unconditionally forced a distinct credential
name, with no way to intentionally share a credential across instances
(e.g. two Anthropic instances -- one pinned to Opus, one to Sonnet --
that both authenticate with the same account/key).

This offers a reuse-or-separate choice instead, defaulting to reuse:

- Reuse (default): binds the new instance to the SAME ${ENV_VAR} as the
  existing instance. The secret field is never prompted for or
  overwritten -- only the placeholder is persisted, so both instances
  resolve the same stored credential at runtime.
- Separate: derives/validates a distinct env var (unchanged from
  today's collision flow) and may be left blank to persist the
  placeholder UNSET for runtime injection (shell / CI / DTU
  passthrough), or populated through the existing secure-storage flow
  (KeyManager.save_key), unchanged.
- Editing an existing instance recovers its binding from the stored
  placeholder, including "shared" when another configured instance
  still references the same env var -- the reuse invariant survives
  edits, not just the initial add.
- Non-interactive mode carries the same semantics: an explicit binding
  (shared or separate) is exempt from the existing collision fail-loud
  guard, since it is intentional rather than an accidental default-name
  collision.
- Runtime hardening: a provider instance's required secret placeholder
  that resolves to unset now fails loudly before session mount, instead
  of silently expanding to "" and letting the provider module fall back
  to its own ambient credential (which would route a "separate"
  instance through a DIFFERENT account's key). Optional/keyless secret
  fields are unaffected.

No settings schema, amplifier-core, or provider-module contract
changes -- this is app-CLI wizard/runtime policy only.

Tests cover all four spec paths (shared / separate / unset-for-runtime-
injection / edit-preservation) plus the non-interactive parity and
runtime fallback-hardening cases. Full suite: 1469 passed, 1 skipped,
13 deselected, 1 xfailed.

This design was independently validated across 10 reference
implementations produced in an internal first-turn evaluation of this
exact issue; this change follows the most consistent design among them
(a named CredentialDecision/binding-mode contract) and additionally
folds in the runtime unset-fallback hardening one of those
implementations added after adversarial review.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach

Copy link
Copy Markdown
Collaborator Author

Admin merge — self-authored PR, merged at explicit user direction.

This PR cannot go through the normal self-approval flow (GitHub prevents authors from approving their own PRs). Per the established maintainer admin-merge pattern (precedent: gemini#39, foundation#317–#320, all merged this way this week), this is being merged directly at the user's explicit direction, with the rationale documented here for the record.

Basis for merge:

  • Implements the validated spec: reuse-default flow, same-${ENV_VAR} persistence with no prompt/overwrite, separate-saved-UNSET for runtime injection, secure-storage path unchanged, edit-preservation.
  • Adds runtime fail-loud hardening: an unset separate binding never silently falls back to a shared credential.
  • Design validated by 10 independent eval implementations.
  • Full suite: 1469 passed; 71 tests across the new/updated credential test files.
  • No core/provider/settings contract changes.
  • Known conflict-risk vs draft feat(cli): full-screen interactive TUI runtime #234 is already noted in the PR body.

CI: all checks green (pytest across ubuntu/macos/windows × py3.11/3.12, integration suites, license/cla).

Merging via gh pr merge --squash --delete-branch=false.

@bkrabach
Brian Krabach (bkrabach) merged commit f7a4ce9 into main Aug 27, 2026
9 checks passed
Brian Krabach (bkrabach) pushed a commit that referenced this pull request Aug 28, 2026
Inline /amplifier-config skill (root provider selection vs spawned-session routing vs bundle composition vs settings-scope precedence): inspects effective config with provenance, proposes smallest safe change with blast-radius/rollback, applies only authorized edits.

Verified against this week's changes (#284 wizard None-default rendering, provider-openai#69 model-gated ConfigFields, #281 multi-instance credential flow) -- none hardcoded in SKILL.md; it correctly defers version-sensitive facts to live inspection + app-cli:cli-expert. Wiring confirmed via auto-discovery in _ensure_default_skills_dirs() (same mechanism as goalify/goal-batch/ten-lane-highway) and new tests (test_packaged_amplifier_config_discovery_and_invocation, test_amplifier_config_skill_contract). Full suite: 1493 passed (main's 1491 + 2 new tests), zero regressions.

Admin-merge at team direction: fork PR (no push access to author's branch to rebase), ruleset requires 1 approving review with none yet recorded, CI 9/9 green, content review complete -- see PR comment for full due-diligence writeup.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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