Skip to content

feat: implement PackageMetadata and set as FoundryContext.metadata#33

Open
olivermeyer wants to merge 2 commits intomainfrom
feat/api-core
Open

feat: implement PackageMetadata and set as FoundryContext.metadata#33
olivermeyer wants to merge 2 commits intomainfrom
feat/api-core

Conversation

@olivermeyer
Copy link
Copy Markdown
Collaborator

  • Implementing a PackageMetadata model to hold package metadata, and setting it as FoundryContext.metadata.
  • Simplifying api.core functions to use the context instead of explicit parameters

Introduces PackageMetadata (frozen Pydantic model) with description,
author_name, author_email, repository_url, and documentation_url fields.
PackageMetadata.from_name() populates all five from importlib.metadata.

FoundryContext.from_package() now delegates to PackageMetadata.from_name()
and exposes the result as ctx.metadata; the top-level repository_url /
documentation_url fields are removed from FoundryContext.

All callers (user_agent, sentry) updated to read from ctx.metadata.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@olivermeyer olivermeyer requested a review from a team as a code owner April 2, 2026 15:43
@olivermeyer olivermeyer added the claude Trigger Claude Code automation label Apr 2, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 2, 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 98.09% <100.00%> (+0.07%) ⬆️
src/aignostics_foundry_core/foundry.py 97.43% <100.00%> (+0.31%) ⬆️
src/aignostics_foundry_core/sentry.py 95.18% <ø> (ø)
src/aignostics_foundry_core/user_agent.py 100.00% <100.00%> (ø)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Apr 2, 2026

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


PR Review Complete ✅

Reviewing PR #33: feat: implement PackageMetadata and set as FoundryContext.metadata


Summary

This PR successfully implements PackageMetadata and simplifies API core functions to derive metadata from context. The changes are well-structured, thoroughly tested, and maintain backward compatibility where appropriate.

Quality Gates: ✅ All passed

  • SonarQube Quality Gate: Passed
  • Code Coverage: 100% on new code
  • No new security hotspots or code duplication

Review Findings

✅ Strengths

  1. Excellent Design

    • Clean separation of concerns with PackageMetadata model
    • Frozen Pydantic models prevent accidental mutations
    • Context-based metadata derivation eliminates parameter threading
  2. Comprehensive Test Coverage

    • 100% coverage on new code (verified by Codecov)
    • Tests properly use @pytest.mark.unit markers
    • Tests follow SonarQube S1192 compliance (string constants extracted)
    • Good test naming: test_package_metadata_from_name_description()
  3. Documentation

    • AGENTS.md thoroughly updated with new API signatures
    • Docstrings are complete and follow Google style
    • Clear migration path in code comments
  4. Code Quality

    • Type hints are complete and correct
    • No SonarQube issues
    • Follows project conventions

🟡 Observations (Non-blocking)

  1. Breaking Changes in api.core (src/aignostics_foundry_core/api/core.py)

    The following functions have breaking signature changes:

    • build_api_metadata(): Previously accepted 7 explicit parameters (title, description, etc.), now takes version and context
    • build_versioned_api_tags(): Previously (version_name, repository_url), now (version_name, *, context=None)
    • get_versioned_api_instances(): Removed build_metadata parameter

    Impact: Any external code calling these functions will break. However, this appears intentional as part of the refactor to context-based metadata.

    Recommendation: This is acceptable since:

    • The package is in active development (v0.7.x)
    • The new API is cleaner and more consistent
    • Migration is straightforward (just remove the explicit parameters)
    • No deprecation warnings needed at this stage
  2. user_agent() signature change (src/aignostics_foundry_core/user_agent.py:12)

    Changed from:

    def user_agent(project_name: str, version: str, repository_url: str) -> str:

    To:

    def user_agent(*, context: "FoundryContext | None" = None) -> str:

    Impact: Breaking change for any external callers.

    Recommendation: This is also acceptable and consistent with the context-driven approach. The new signature is more maintainable.

  3. TODO Comment (src/aignostics_foundry_core/user_agent.py:43)

    # TODO(oliverm): Find a way to not hard code python-sdk here. This was taken as such from Bridge.

    Observation: This is a known issue carried over from Bridge. Not blocking for this PR, but worth tracking.

📝 Suggestions (Optional Improvements)

  1. Consider a CHANGELOG entry

    Since this PR introduces breaking changes to public API functions, consider adding a CHANGELOG entry highlighting:

    • New PackageMetadata model
    • Breaking changes in api.core function signatures
    • Migration guide
  2. Package metadata parsing robustness (src/aignostics_foundry_core/foundry.py:72-73)

    author_name = author.split("<")[0].strip() if author else None
    author_email = author.split("<")[1].strip(" >") if author else None

    Current behavior: If author exists but doesn't contain <, this will raise an IndexError.

    Suggestion: Add defensive parsing:

    if author and "<" in author:
        author_name = author.split("<")[0].strip()
        author_email = author.split("<")[1].strip(" >")
    else:
        author_name = author.strip() if author else None
        author_email = None

    Severity: Low (package metadata is typically well-formed)

  3. Test isolation (tests/conftest.py:24)

    The make_context() helper defaults to:

    metadata=metadata or PackageMetadata()

    This is good! It ensures tests don't depend on real package metadata. Well done.


Security Review ✅

  • No hardcoded secrets
  • No SQL injection vectors (Pydantic models, no raw queries)
  • No XSS risks (server-side code only)
  • URL parsing in PackageMetadata.from_name() is safe (reads from package metadata, not user input)

Performance Review ✅

  • PackageMetadata.from_name() is called once during FoundryContext.from_package() at startup
  • No performance regressions
  • Frozen models are slightly faster than mutable ones

Commit Messages ✅

Both commits follow conventional commit format:

  • feat(foundry): add PackageMetadata to FoundryContext
  • refactor(api): derive metadata from context in api.core helpers

Clean, descriptive, and properly scoped.


Verdict

✅ APPROVE - This PR is ready to merge.

The code is high quality, well-tested, and follows all project standards. The breaking changes are intentional, well-scoped, and improve the API design. SonarQube and Codecov both pass with flying colors.

Optional Follow-ups (Post-merge)

  1. Address the TODO in user_agent.py about hardcoded "python-sdk"
  2. Consider adding defensive parsing for author email extraction (low priority)
  3. Update CHANGELOG with breaking changes (if maintaining one)

Great work! 🎉 This refactor significantly improves the API ergonomics and reduces boilerplate.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 2, 2026

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