Plan #19: env vars override the YAML config file, as documented - #62
Merged
Merged
Conversation
Triage of the AI-drafted ticket and the implementation plan. The diagnosis holds and still applies in 1.16.0. The plan adds what the draft skipped: how a settings source learns the YAML path (a ContextVar-backed source set by from_yaml(), prototyped), the silent retargeting of deployments whose env and YAML disagree (a NEXT.md entry), the tracked root mldp-config.yaml's effect on test isolation, and the wider doc and test surface. Q1 (config_object returned as-is) and Q2 (no override logging) are resolved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsRVNfPSn4SG7UiU7m7hQq
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two moderate validation issues and one release-notes quality-gate issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR adds the implementation plan for fixing YAML/environment-variable precedence in configuration loading.
Changes:
- Documents the ContextVar-backed YAML settings source design.
- Defines implementation, testing, documentation, and release-note tasks.
- Clarifies configuration-object behavior and rollout considerations.
Review findings require updates: add an identity assertion, test direct init-versus-environment precedence, and include the release-notes checker in quality gates.
| File | Summary |
|---|---|
plan/tickets/19/plan.md |
Triage, design, implementation tasks, and validation plan for issue #19. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ternative - Task 3: build the explicit config inside the patched environment, and assert identity, so the level-1 test cannot pass against the old rebuild (Copilot review). - Task 8: add the cookbook and release-notes checkers to the quality gates. - D1: record the model_fields_set filter as a rejected alternative, and why. - Task 2: switch the config_object guard to `is not None`. - T7 line reference; T6 notes that existing tests stay unisolated (out of scope). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsRVNfPSn4SG7UiU7m7hQq
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
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.
Refs #19. This PR contains only the plan; the fix comes in a follow-up PR. I used
Refsrather thanClosesso the issue stays open until then.Adds
plan/tickets/19/plan.md, the triage and implementation plan for #19, for review before any code changes.Summary
MldpConfig.from_yaml()passes YAML values as init kwargs, which pydantic-settings ranks aboveMLDP_*env vars. 1.16.0 shipped with the bug; the config code is unchanged since 1.15.0.from_yaml()passes its flattened values to that source through aContextVar. I prototyped this against pydantic-settings 2.14.2 and every case matches the documented order.NEXT.mdentry.mldp-config.yamlsets every key, which affects test isolation.load_config(config_object=)returns the object as-is.Review focus
🤖 Generated with Claude Code
https://claude.ai/code/session_01PsRVNfPSn4SG7UiU7m7hQq