refactor(config): Two-Medium architecture -- collapse scalar YAML to … - #44
Merged
Mowri Mohan (mowree) merged 6 commits intoApr 13, 2026
Conversation
…Python dataclasses Follow-up to PR microsoft#32 review by @samueljklee: "retry.yaml (4 values), model_cache.yaml (1 value), sdk_protection.yaml (safety invariants) are over-engineered for a leaf provider module." Architecture change -- Two-Medium replaces Three-Medium: Python = policy + mechanism (config/_models.py, _policy.py, _sdk_protection.py) YAML = SDK-correlated tabular data only (config/data/errors.yaml, events.yaml) Collapsed 6 scalar YAML files to frozen Python dataclasses: config/retry.yaml -> RetryPolicy in config/_policy.py config/model_cache.yaml -> CacheConfig in config/_policy.py config/sdk_protection.yaml -> SdkProtectionPolicy in config/_sdk_protection.py config/fake-tool-detection.yaml -> LoggingConfig in config/_policy.py config/models.yaml -> ProviderIdentity in config/_models.py config/observability.yaml -> constants in observability.py Preserved as YAML (moved to config/data/ for importlib.resources): config/data/errors.yaml -- 14 SDK->kernel mappings (genuinely tabular) config/data/events.yaml -- 40+ event classifications (SDK-correlated) Test fixes: - 30+ test files: Three-Medium/models.yaml/retry.yaml refs -> Two-Medium - test_live_smoke.py: remove 2 permanently-skipping tests (send_and_wait and message_delta gated by org policy not available on this account) - test_sdk_assumptions.py: remove 2 dead SubprocessConfig tests (SubprocessConfig removed from SDK v0.2.x) - test_unified_error_config.py: fix stale path config/errors.yaml -> config/data/errors.yaml - conftest.py, test_cross_platform.py: pyright: ignore for Windows-only asyncio.WindowsSelectorEventLoopPolicy Repo hygiene: - .gitignore: align section order/style with microsoft/amplifier ecosystem - Makefile: --cov-branch, threshold 80->90%, add make live target - scripts/smoke_test.py: fix stale count, fix non-existent test_provider.py refs - scripts/amplifier_settings.yaml: remove (personal dev config leaked in) - README.md: fix stale RUN_LIVE_TESTS env var, fix tests/integration/ path - pyrightconfig.example.json: add contributor template Quality: Windows (Python 3.13): 1186 passed, 3 skipped, 97% branch coverage WSL (Python 3.14): 1188 passed, 1 skipped, 97% branch coverage ruff: 0 errors | pyright: 0 errors | all files >= 91% coverage
safe_log_message() redacted variadic args but returned the message string verbatim -- f-string callers bypassed all redaction silently. PEM headers survived the opaque-token pattern. Database passwords in connection URIs had no coverage. - Apply redact_sensitive_text() to message in safe_log_message() - Add _PEM_BLOCK_PATTERN (re.DOTALL) applied before opaque-token pass - Add _DB_URI_PATTERN -- redacts password segment only, preserves host - Register both in _count_secrets() for idempotency Tests: +10 | 1201 passed, 0 failed | security_redaction.py 100% | ruff 0 | pyright 0
…on check
SDK delivers tool arguments as dict or JSON string; isinstance() guard silently
returned {} on strings. Cache reader discarded all entries on one bad record.
Cache version was written but never validated on read.
- _parse_tool_arguments(): dict passthrough, json.loads for strings, {} for None
- Per-entry loop in read_cache() -- bad entries skipped, logged by index (SFI safe)
- All-malformed cache returns None (cache miss), not [] (no models)
- Version validated before parsing; mismatch -> None; missing -> "1.0" compat
Tests: +12 | 1201 passed, 0 failed | 97% branch coverage | ruff 0 | pyright 0
…th placeholder
Image blocks fell through _extract_content_block() to the text-probe fallback,
matched nothing, and returned "" -- the model received no context that an image
existed in that turn.
- Explicit branch for block_type == "image" and duck-typed image objects
(hasattr url or source, no text/output attribute)
- Returns "[Image: {mime}]" or "[Image]"; WARNING logged on every substitution
Tests: +6 | 1201 passed, 0 failed | request_adapter.py 97% | ruff 0 | pyright 0
Call site in to_chat_response() kept the original isinstance guard after
_parse_tool_arguments() was introduced. JSON-string arguments produced {}
with no exception, no log entry.
- Replace isinstance guard with _parse_tool_arguments(tc.get("arguments"))
- Integration tests confirmed red before this change, green after
Tests: +2 end-to-end | 1201 passed, 0 failed | streaming.py 99% | ruff 0 | pyright 0
Importing CopilotClientWrapper pulled error_translation and security_redaction as module-level side effects. Any code that imported the sdk_adapter layer also imported the domain error system and security utilities \u2014 coupling the isolation membrane to the modules it is supposed to wrap. - Remove module-level imports of translate_sdk_error and safe_log_message - Add _translate_error and _safe_log to CopilotClientWrapper.__init__ (keyword-only, underscore-prefixed internal API; backward-compatible defaults) - Lazy resolvers _get_translate_error() and _get_safe_log() load the real functions on first call; injected callables short-circuit the lazy load - Replace ConfigurationError / ProviderUnavailableError lazy imports (from\n ..error_translation re-export) with direct amplifier_core.llm_errors imports\n at module level \u2014 correct provenance for kernel types - _load_error_config_once() return type widened to Any; ErrorConfig imported\n lazily inside the function alongside load_error_config Tests: +7 in test_sdk_client_injection.py - injected translator called on session error - injected translator called on list_models error - default translator still produces LLMError (no regression) - injected safe_log called on disconnect failure - injected safe_log called on close failure - ConfigurationError identity: amplifier_core.llm_errors (not re-export) - ProviderUnavailableError identity: amplifier_core.llm_errors (not re-export) 1224 passed, 3 skipped, 0 failed | ruff 0 | pyright 0
Mowri Mohan (HDMowri)
added a commit
to HDMowri/amplifier-module-provider-github-copilot
that referenced
this pull request
Apr 14, 2026
Fix _extract_event_data: data=None no longer leaks into DomainEvent.data
(session.idle events were producing spurious {data: None} entries).
Tests: spec= on all mocks, exact-value assertions replace is not None,
inspect.getsource checks replaced with behavioral equivalents. Deleted
test_sdk_api_conformance.py and test_timeout_alignment.py (YAML config
gone in PR microsoft#44). New: test_sdk_client_injection.py, flatten regression
guard, malformed JSON tool args resilience.
Contracts: sdk-boundary, event-vocabulary, behaviors, provider-protocol
updated to reflect dataclass-based config and new invariants.
Gates: 1157 passed (Windows, all tests incl. live), 2 xfailed (Unix
chmod -- expected), 0 deselected. WSL: 1159 passed, 98% branch coverage.
ruff: 0 errors. pyright: 0 errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…Python dataclasses
Follow-up to PR #32 review by Samuel Lee (@samueljklee):
"retry.yaml (4 values), model_cache.yaml (1 value), sdk_protection.yaml
(safety invariants) are over-engineered for a leaf provider module."
Architecture change -- Two-Medium replaces Three-Medium:
Python = policy + mechanism (config/_models.py, _policy.py, _sdk_protection.py)
YAML = SDK-correlated tabular data only (config/data/errors.yaml, events.yaml)
Collapsed 6 scalar YAML files to frozen Python dataclasses:
config/retry.yaml -> RetryPolicy in config/_policy.py
config/model_cache.yaml -> CacheConfig in config/_policy.py
config/sdk_protection.yaml -> SdkProtectionPolicy in config/_sdk_protection.py
config/fake-tool-detection.yaml -> LoggingConfig in config/_policy.py
config/models.yaml -> ProviderIdentity in config/_models.py
config/observability.yaml -> constants in observability.py
Preserved as YAML (moved to config/data/ for importlib.resources):
config/data/errors.yaml -- 14 SDK->kernel mappings (genuinely tabular)
config/data/events.yaml -- 40+ event classifications (SDK-correlated)
Test fixes:
Repo hygiene:
Quality:
Windows (Python 3.13): 1186 passed, 3 skipped, 97% branch coverage
WSL (Python 3.14): 1188 passed, 1 skipped, 97% branch coverage
ruff: 0 errors | pyright: 0 errors | all files >= 91% coverage