Skip to content

feat: implement project context injection#16

Merged
olivermeyer merged 3 commits into
mainfrom
feat/project-context-injection
Mar 27, 2026
Merged

feat: implement project context injection#16
olivermeyer merged 3 commits into
mainfrom
feat/project-context-injection

Conversation

@olivermeyer
Copy link
Copy Markdown
Collaborator

@olivermeyer olivermeyer commented Mar 27, 2026

  • Adds docs/decisions/0003-project-context-injection.md
  • Implements the context code in foundry.py
  • Updates di.py and all downstream call sites to use the context code

I will update the remaining modules in a follow-up PR to keep this one somewhat manageable.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 27, 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/api/core.py 97.89% <100.00%> (+0.09%) ⬆️
src/aignostics_foundry_core/cli.py 78.04% <100.00%> (+1.73%) ⬆️
src/aignostics_foundry_core/di.py 100.00% <100.00%> (ø)
src/aignostics_foundry_core/foundry.py 100.00% <100.00%> (ø)
src/aignostics_foundry_core/gui/core.py 97.91% <100.00%> (ø)
src/aignostics_foundry_core/gui/nav.py 100.00% <100.00%> (ø)

@arne-aignx
Copy link
Copy Markdown

My main concern here is overly relying on pydantic for something that a dataclass can do.
pydantic shines, where it does input validation (JSON, environment variables etc.). Here, it is used as a simple DTO.

To put this into broader context (I checked the ADRs, which you recently introduced, and did not find anything)

IMO: A core library should be as lightweight as possible. When we include on dependencies, we should do so with care.
Also dependencies should be extras if possible (e.g. NiceGUI, FastAPI, FastMCP, typer might not be required by every library).

If we consider pydantic and pydantic-setttings as part of the "true" core, it should be documented in another, and can be freely used to implement this ADR.

That said:
LGTM - I am not blocking here, but I want to point out some peculiarities with core libraries.
I have some PTSD from a library called python-common-utilities with far to many dependencies

@kruegener
Copy link
Copy Markdown
Member

Agreeing with @arne-aignx's comment. I think the company as a whole has aligned on pydantic as the dataclass wrapper, even if regular dataclasses may suffice in many cases. So I don't see that particular one as a blocker and this ADR is supported as-is by me.
However I checked https://github.com/aignostics/foundry-python-core/blob/main/pyproject.toml and you added e.g. nicegui to the default dependencies of the foundry core @olivermeyer just now. Please consider moving this and other libraries into extras as also requested by Tom. My gut check for if a package should be included in the default list of dependencies is "is it needed to run the CLI version of the Service", as that in my head is the smallest somewhat usable thing one can build on top of the core.

@olivermeyer
Copy link
Copy Markdown
Collaborator Author

RE Pydantic vs dataclass: I would agree with using a dataclass if FoundryContext was a plain DTO with no logic, however in this case it has a non-trivial constructor and could therefore benefit from Pydantic's validation. I pushed my current WIP for illustration, see from_package() here.

RE making remaining packages optional: I believe this needs some further discussion. In any case, I would do that as a follow-up task.

@arne-aignx
Copy link
Copy Markdown

LGTM.
Let's discuss the packages at another time

olivermeyer and others added 2 commits March 27, 2026 14:47
@olivermeyer olivermeyer force-pushed the feat/project-context-injection branch from e96538f to 7817326 Compare March 27, 2026 14:46
@olivermeyer olivermeyer added the claude Trigger Claude Code automation label Mar 27, 2026
@olivermeyer olivermeyer marked this pull request as ready for review March 27, 2026 14:51
@olivermeyer olivermeyer requested a review from a team as a code owner March 27, 2026 14:51
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@olivermeyer olivermeyer force-pushed the feat/project-context-injection branch from 7817326 to 6f1bac4 Compare March 27, 2026 14:51
@claude
Copy link
Copy Markdown

claude Bot commented Mar 27, 2026

Claude finished @olivermeyer's task in 3m 14s —— View job


Code Review Complete ✓

I've completed a thorough review of PR #16. Overall, this is excellent work with strong architecture, comprehensive testing, and good documentation. The implementation follows best practices and addresses the team's feedback about pydantic usage.


Summary

Verdict: ✅ Approved with minor suggestions

This PR successfully implements project context injection via FoundryContext, eliminating the need for per-project _constants.py files. The ADR is well-written, the implementation is clean, and test coverage is 100%.


Review Breakdown

Strengths

  1. Excellent ADR (0003) - Clear problem statement, well-reasoned decision process, concrete examples
  2. Immutable design - Frozen Pydantic models prevent accidental mutation
  3. Test-friendly - Explicit context override avoids global state pollution in tests
  4. 100% test coverage - All new code is thoroughly tested with proper markers
  5. Clean API - Simple set_context() / get_context() pattern familiar from logging libraries
  6. SonarQube compliance - Tests extract constants properly (S1192)
  7. Type safety - Proper TYPE_CHECKING blocks, no runtime circular imports

📋 Suggestions (Non-blocking)

1. Fragile CLI detection logic (foundry.py:192)

Location: src/aignostics_foundry_core/foundry.py:192

Issue: The is_cli detection relies on sys.argv[0].endswith(name) or sys.argv[1] == name, which could fail in edge cases (e.g., when invoked via python -m, when argv is manipulated, or when the script name doesn't match the package name).

is_cli = sys.argv[0].endswith(name) or (len(sys.argv) > 1 and sys.argv[1] == name)

Suggestion: Consider adding a comment documenting known edge cases, or use a more robust detection method:

# Detect CLI mode: matches if script name ends with package name OR if first arg is package name
# Note: May not detect python -m invocations or unusual entry points
is_cli = sys.argv[0].endswith(name) or (len(sys.argv) > 1 and sys.argv[1] == name)

Why: Makes limitations explicit for future maintainers.


2. Complex version_full format (foundry.py:111-142)

Location: src/aignostics_foundry_core/foundry.py:111-142

Issue: The _build_version_full function constructs a complex version string with +, -, and --- separators, but the format isn't documented.

Suggestion: Add a docstring example showing the output format:

def _build_version_full(version: str) -> str:
    """Append build metadata to *version* from environment variables.
    
    Format: {version}+{vcs_ref}-{commit_sha}---{extra}---{extra}...
    Example: "1.0.0+abc123-deadbeef---run.99---build.42---builder.uv---built.2024-01-15"
    
    Returns:
        The version string with optional ``+<metadata>`` suffix.
    """

Why: Improves maintainability and helps users understand what to expect in logs.


3. No package validation (foundry.py:71)

Location: src/aignostics_foundry_core/foundry.py:71

Issue: from_package(package_name) doesn't validate that package_name is importable before calling metadata.version(), which will raise PackageNotFoundError if the package doesn't exist.

Suggestion: This is probably acceptable (fail-fast is good), but consider adding a note in the docstring:

Args:
    package_name: The importable package name (e.g. ``"bridge"``).
        Must be installed and visible to importlib.metadata.

Raises:
    PackageNotFoundError: If package_name is not installed.

Why: Makes the failure mode explicit.


4. Env file paths (foundry.py:154-169)

Location: src/aignostics_foundry_core/foundry.py:154-169

Issue: The env_file list always includes 5 paths, but in practice only user home paths are likely to exist. This is not a bug, but worth noting.

Current behavior: Returns all paths regardless of existence.

Suggestion: Add a comment explaining that existence checking happens at usage time, not construction time:

def _build_env_file_list(name: str, name_upper: str, environment: str) -> list[Path]:
    """Build the ordered list of env files for *name* in *environment*.
    
    Returns all candidate paths regardless of existence. Consumers are
    responsible for checking which files exist.
    
    Returns:
        Ordered list of candidate env-file paths.
    """

Why: Clarifies design intent.


🔍 Code Quality Checks

Check Status Notes
Type Safety Proper TYPE_CHECKING blocks, no circular imports
Security No hardcoded secrets, safe environment variable handling
Performance Appropriate caching in di.py, frozen models prevent copies
Testing 100% coverage, all tests have markers, good fixture usage
SonarQube Quality gate passed, constants extracted per S1192
Documentation Comprehensive ADR, good docstrings, AGENTS.md updated
Breaking Changes API change in di.py (project_name → context), but internal only

📝 Testing Quality

Test coverage: ✅ 100% on new code (Codecov)
Test markers: ✅ All tests have @pytest.mark.unit
Test quality: ✅ Excellent

  • Comprehensive coverage of from_package() logic
  • Environment variable testing with monkeypatch
  • Immutability testing
  • Global state reset via reset_context fixture
  • Constants extracted per SonarQube S1192
  • Good use of make_context() helper in conftest.py

Example of good test structure (foundry_test.py):

@pytest.mark.unit
def test_from_package_version_full_joins_vcs_ref_and_commit_sha_with_dash(monkeypatch: pytest.MonkeyPatch) -> None:
    """version_full joins VCS_REF and COMMIT_SHA with '-' when both are set."""
    monkeypatch.setenv("VCS_REF", VCS_REF_VALUE)
    monkeypatch.setenv("COMMIT_SHA", COMMIT_SHA_VALUE)
    ctx = FoundryContext.from_package(PACKAGE_NAME)
    assert f"{VCS_REF_VALUE}-{COMMIT_SHA_VALUE}" in ctx.version_full

🎯 Architecture Review

Design Pattern: ✅ Well-chosen (Option #5 from ADR)

The combination of:

  • FoundryContext.from_package() - Centralized derivation logic
  • set_context() / get_context() - Library-level state
  • Explicit context override for tests

...is the right balance between ergonomics (production) and testability.

Pydantic usage: ✅ Justified

The discussion in PR comments about pydantic vs dataclass is resolved correctly. Pydantic provides:

  1. Validation (important for from_package() complexity)
  2. Frozen models (immutability)
  3. model_dump() for subclassing (see ADR "Extending FoundryContext")

Global state: ✅ Acceptable

The global _context variable is acceptable because:

  • Set once at startup
  • Tests use explicit override (no reset needed)
  • Clear error message if not initialized
  • Documented in ADR

🔐 Security Review

✅ No issues found

  • Environment variables are read, not written
  • No secrets in code
  • No injection vulnerabilities
  • Safe Path construction

Performance Review

✅ No concerns

  • Frozen Pydantic models (no copy overhead)
  • Appropriate caching in di.py (keyed by context name)
  • from_package() only called once at startup
  • get_context() is a simple global lookup

📚 Documentation Review

✅ Excellent documentation

  • ADR 0003: Comprehensive, well-structured, includes rationale and examples
  • Docstrings: Complete with examples, param descriptions, and cross-references
  • AGENTS.md: Updated with new foundry module documentation
  • Type hints: Complete and accurate

Recommendations

Before Merge (Optional)

  1. Consider adding docstring examples to _build_version_full and _build_env_file_list
  2. Consider documenting is_cli detection edge cases

Follow-up Work (As mentioned in PR description)

  1. Update remaining modules to use context (planned for follow-up PR)
  2. Discuss making optional dependencies (nicegui, etc.) into extras (per team feedback)

Final Assessment

Quality: ⭐⭐⭐⭐⭐ (5/5)
Test Coverage: 100%
Documentation: Excellent
Architecture: Sound
Ready to Merge: ✅ Yes

This PR demonstrates excellent software engineering:

  • Thoughtful design documented in ADR
  • Clean, type-safe implementation
  • Comprehensive testing
  • Clear documentation
  • Team feedback incorporated

Great work, @olivermeyer! 🎉


Tasks

  • Analyze git diff and commit history
  • Review ADR (0003-project-context-injection.md)
  • Review implementation (foundry.py)
  • Review DI changes (di.py)
  • Review test coverage and quality
  • Review usage in API/CLI modules
  • Check for security issues
  • Check for performance concerns
  • Verify documentation completeness
  • Provide structured feedback

@sonarqubecloud
Copy link
Copy Markdown

@olivermeyer olivermeyer merged commit 4959a59 into main Mar 27, 2026
15 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.

3 participants