Skip to content

feat(settings): add OpaqueSettings and load_settings#7

Merged
olivermeyer merged 4 commits intomainfrom
feat/settings-module
Mar 25, 2026
Merged

feat(settings): add OpaqueSettings and load_settings#7
olivermeyer merged 4 commits intomainfrom
feat/settings-module

Conversation

@olivermeyer
Copy link
Copy Markdown
Collaborator

Porting the settings module from Bridge.

@olivermeyer olivermeyer requested a review from a team as a code owner March 24, 2026 15:36
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines Coverage Δ
src/aignostics_foundry_core/settings.py 100.00% <100.00%> (ø)

@olivermeyer olivermeyer added the claude Trigger Claude Code automation label Mar 24, 2026
Port OpaqueSettings, load_settings, strip_to_none_before_validator,
and UNHIDE_SENSITIVE_INFO from bridge into foundry-python-core so
downstream Foundry components can use shared settings infrastructure
without coupling to bridge.

- Add pydantic-settings>=2,<3 runtime dependency
- Create aignostics_foundry_core/settings.py with the four public names
- 14 unit tests with 100% branch coverage
- Update AGENTS.md module index with settings entry

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@olivermeyer olivermeyer force-pushed the feat/settings-module branch from 43d9e25 to 1c9324a Compare March 24, 2026 16:21
@claude
Copy link
Copy Markdown

claude bot commented Mar 24, 2026

Claude finished @olivermeyer's task in 2m 52s —— View job


PR Review Complete ✅

Comprehensive code review for PR #7: feat(settings): add OpaqueSettings and load_settings

Tasks

  • Read project documentation (CLAUDE.md, CODE_STYLE.md)
  • Analyze PR changes via git diff
  • Review code quality and style compliance
  • Verify test coverage and markers
  • Check for security issues
  • Review documentation completeness
  • Verify conventional commit compliance
  • Post review findings

✅ Review Summary

Excellent work! This is high-quality, production-ready code that meets all project standards. The settings module is well-designed, thoroughly tested, and properly documented.

Code Quality: Excellent ✅

Strengths:

  • 100% test coverage - All new code fully covered (confirmed by Codecov)
  • SonarQube quality gate passed - Zero violations, zero security hotspots
  • Type safety - Comprehensive type hints throughout, proper use of TypeVar
  • Documentation - Google-style docstrings on all functions, AGENTS.md updated with detailed module description
  • Security - Proper secret masking via serialize_sensitive_info, test fixtures appropriately use # noqa: S105
  • Error handling - User-friendly Rich-formatted validation errors, semantically correct exit code 78 (EX_CONFIG)

Test Quality: Excellent ✅

All tests properly marked: Every test has @pytest.mark.unit

Comprehensive edge case coverage:

  • src/aignostics_foundry_core/settings.py:19-33 - strip_to_none_before_validator: Tests for None, empty string, whitespace, and valid input
  • src/aignostics_foundry_core/settings.py:40-55 - serialize_sensitive_info: Tests masking/unmasking, None/empty secrets, missing context
  • src/aignostics_foundry_core/settings.py:58-71 - serialize_path_resolve: Tests path resolution, None input, and empty Path() (prevents CWD resolution)
  • src/aignostics_foundry_core/settings.py:74-122 - load_settings: Tests success cases, env prefixes, validation errors, and integer loc indices (RootModel edge case)

Notable test: tests/aignostics_foundry_core/settings_test.py:162-183 excellently validates the defensive programming in src/aignostics_foundry_core/settings.py:98 that handles non-string error locations (prevents nonsensical "PREFIX_0" environment variable names).

Code Design: Excellent ✅

Defensive programming highlights:

  • src/aignostics_foundry_core/settings.py:69 - Empty Path check (not input_value.parts) prevents resolving Path("") to CWD
  • src/aignostics_foundry_core/settings.py:98 - Type guard for error loc indices handles RootModel validation errors gracefully
  • src/aignostics_foundry_core/settings.py:109 - Dual fallback (get("env_file", ".env") or ".env") handles None/empty explicitly

Documentation: Excellent ✅

  • Module docstring present
  • Function docstrings - All functions documented with Google-style Args/Returns/Raises
  • AGENTS.md updated - Comprehensive module description with purpose, key features, dependencies
  • License attribution - ATTRIBUTIONS.md properly updated for pydantic-settings

Conventional Commits: ✅

PR title: feat(settings): add OpaqueSettings and load_settings ✓ Follows conventional commits

Commit messages:

  • 5fd9178 - fix(settings): handle non-string loc in validation error formatting
  • 490b1b3 - fix(settings): use explicit None/empty guard for Path serializer
  • 1c9324a - feat(settings): add OpaqueSettings and load_settings
  • 11f72b1 - fix: review comments - Generic but will be squashed ⚠️

Security: ✅

  • ✅ No hardcoded secrets (test fixture _SECRET_VALUE appropriately marked with # noqa: S105)
  • ✅ Secret masking implemented correctly
  • ✅ Path resolution uses .resolve() (safe, no traversal issues)
  • ✅ Input validation via Pydantic

No Issues Found

Blocking issues: None

Suggestions: None - this code is ready to merge


Recommendation: APPROVE

This PR demonstrates excellent engineering practices:

  • Clean, well-tested code
  • Comprehensive documentation
  • Proper security considerations
  • Follows all project conventions

The settings module provides valuable reusable infrastructure for the Foundry ecosystem with user-friendly error messages and proper secret handling.


olivermeyer and others added 3 commits March 24, 2026 17:29
`Path` has no `__bool__`, making all instances truthy. The previous
`if not input_value:` guard was therefore never entered for any `Path`,
so `Path("")` silently resolved to the current working directory.

Replace the falsy check with an explicit guard:
  if input_value is None or not input_value.parts:

This correctly returns `None` for both `None` and empty/no-parts paths
(e.g. `Path("")` / `Path()`), while resolving all other paths as before.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
In Pydantic v2, `loc` entries can be integers (e.g. for list-typed fields
validated via RootModel). The previous single-expression format

    f"{prefix}{error['loc'][0]}".upper()

produced meaningless env-var names like "MY_PREFIX_0" for integer locs.

Add an explicit isinstance guard so integer locs fall back to the model
prefix (e.g. "MY_PREFIX") rather than appending the raw integer.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@olivermeyer
Copy link
Copy Markdown
Collaborator Author

RE Claude's blocking issues: #1 is invalid because SecretStr("") is not truthy; #2 and #3 are valid and fixed in 490b1b3 and 5fd9178

@sonarqubecloud
Copy link
Copy Markdown

@olivermeyer olivermeyer merged commit fc25de4 into main Mar 25, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude Trigger Claude Code automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant