Repository navigation
[BREAKING] Python: Refine SecretString handling - #8127
Evan Mattson (moonbox3) merged 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The hashable wrapper remains mutable, and public OpenAI constructor annotations still reject supported SecretString values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refines Python secret handling by making SecretString a masked value wrapper and explicitly unwrapping credentials at provider SDK boundaries.
Changes:
- Adds masked formatting, concatenation, equality, and validation semantics.
- Updates provider credential annotations and SDK integrations.
- Expands tests across supported providers.
File summaries
| File | Description |
|---|---|
python/packages/core/agent_framework/_settings.py |
Implements the new wrapper and coercion. |
python/packages/core/tests/core/test_settings.py |
Tests masking and value semantics. |
python/packages/core/AGENTS.md |
Documents settings conventions. |
python/packages/openai/tests/openai/test_openai_shared.py |
Tests SDK-boundary unwrapping. |
python/packages/mistral/tests/mistral/test_mistral_embedding_client.py |
Tests embedding credentials. |
python/packages/mistral/tests/mistral/test_mistral_chat_client.py |
Tests chat credentials. |
python/packages/gemini/agent_framework_gemini/_chat_client.py |
Accepts wrapped API keys. |
python/packages/gemini/tests/test_gemini_client.py |
Tests Gemini key resolution. |
python/packages/bedrock/agent_framework_bedrock/_chat_client.py |
Updates credential annotations. |
python/packages/bedrock/agent_framework_bedrock/_embedding_client.py |
Updates credential annotations. |
python/packages/bedrock/tests/test_bedrock_client.py |
Tests session credential unwrapping. |
python/packages/azure-cosmos/agent_framework_azure_cosmos/_history_provider.py |
Unwraps Cosmos credentials. |
python/packages/azure-cosmos/agent_framework_azure_cosmos/_checkpoint_storage.py |
Unwraps checkpoint credentials. |
python/packages/azure-cosmos/tests/test_cosmos_history_provider.py |
Tests history credentials. |
python/packages/azure-cosmos/tests/test_cosmos_checkpoint_storage.py |
Tests checkpoint credentials. |
python/packages/azure-ai-search/agent_framework_azure_ai_search/_context_provider.py |
Supports wrapped search keys. |
python/packages/azure-ai-search/tests/test_aisearch_context_provider.py |
Tests search key boundaries. |
python/packages/anthropic/agent_framework_anthropic/_chat_client.py |
Accepts wrapped API keys. |
python/packages/anthropic/agent_framework_anthropic/_foundry_client.py |
Accepts wrapped Foundry keys. |
python/packages/anthropic/agent_framework_anthropic/_bedrock_client.py |
Accepts wrapped AWS credentials. |
python/packages/anthropic/tests/test_anthropic_client.py |
Tests Anthropic key handling. |
python/packages/anthropic/tests/test_anthropic_provider_clients.py |
Tests provider credential boundaries. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 4ceefd0b6ff1
Model: gpt-5.6-sol-fast
Overview
The PR replaces the str subclass with a masking value wrapper and adds explicit unwrapping at most provider SDK boundaries, backed by broad settings and provider tests. The masking, coercion, and direct SDK-boundary guards are strong, but the provider migration is incomplete: Foundry embeddings reject wrapped credentials at runtime, while the public OpenAI client signatures reject them during static checking despite runtime support.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_settings.py
4ceefd0 to
5abc6f0
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
5abc6f0 to
2caf482
Compare
…ecise error Since microsoft#8127 SecretString is an immutable, slots-based non-str wrapper with no __reduce__/__getstate__, so pickle protocols >= 2 dump it fine and fail at restore on the immutability guard. FileCheckpointStorage's save-time round-trip check (microsoft#8214) surfaced that as an opaque decode error at save; Cosmos and Foundry-hosted storage skip that check, so the same state saved cleanly and only failed on resume. Unwrapping to str first, the obvious workaround, writes the credential to durable storage in cleartext. The checkpoint codec now refuses a SecretString at encode time with a WorkflowCheckpointException that says why (unpicklable by design, and a successful pickle would persist the secret in cleartext) and what to do instead (keep credentials out of checkpointed state, re-resolve from settings or configuration on restore). This matches the durable session state path, which already rejects the type with a precise error (microsoft#8874), and because the refusal lives in the shared codec it lands at save time on every storage backend, closing the Cosmos load-time asymmetry. Nested SecretString values inside plain dict/list state are caught by the same recursion. A SecretString buried in a custom object still fails the round-trip check with the generic wrapper; that edge is unchanged.
Motivation & Context
Make settings-value behavior more consistent across Python formatting and provider integrations.
Description & Review Guide
SecretStringbehavior and align provider credential handling and type annotations.SecretStringis now a value wrapper rather than astrsubclass. Callers needing a plain string should useget_secret_value(). Plain-string settings inputs remain supported.Related Issue
N/A.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.