📝 CodeRabbit Chat: Implement requested code changes

[coderabbitai]

Code changes was requested by @leynos.

• Source and presenter-profile intake (4.3.1) #131 (comment)

The following files were modified:

• episodic/canonical/idempotency.py
• episodic/canonical/ingestion_sources.py
• episodic/canonical/uploads.py

Summary by Sourcery

Extract shared validation logic into reusable helper functions across ingestion source, idempotency, and upload domain models.

Enhancements:

• Deduplicate and centralize validation of ingestion job source invariants, including attachment fields and weighting constraints.
• Unify idempotency record and acquire request string field validation and outcome checks through shared helpers.
• Centralize upload size and string field validation for both upload metadata and initialization requests.

[coderabbitai]

Important

Review skipped

This PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b4d9a349-d015-4791-a964-6d6b020e1dba

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sourcery-ai]

Reviewer's Guide

Refactors domain validation logic for ingestion sources, idempotency records/requests, and uploads into reusable private helper functions while preserving existing invariants and error messages.

Flow diagram for shared upload validation helpers

flowchart TD
    Start["Upload validation entrypoints"]

    Start --> UPost["Upload.__post_init__"]
    Start --> IRequestPost["UploadInitRequest.__post_init__"]

    UPost --> DeclaredSizeU["_require_non_negative_size(declared_size, declared_size)"]
    UPost --> ActualSize["_require_non_negative_actual_size(actual_size)"]
    UPost --> ContentTypeU["_require_non_empty_string(content_type, content_type)"]
    UPost --> StorageKey["_require_non_empty_string(storage_key, storage_key)"]

    IRequestPost --> DeclaredSizeR["_require_non_negative_size(declared_size, declared_size)"]
    IRequestPost --> ContentTypeR["_require_non_empty_string(content_type, content_type)"]

Loading

File-Level Changes

Change	Details	Files
Extract shared ingestion source validation logic into dedicated helper functions and reuse them in IngestionJobSource.post_init.	Introduce private helpers to enforce exclusive upload/source URI, attachment-kind-specific presence of identifiers, non-empty source_type, and weight bounds. Replace inline validation in IngestionJobSource.post_init with calls to the new helpers, preserving semantics and error messages.	episodic/canonical/ingestion_sources.py
Centralize idempotency key and completion validation into reusable helpers used by IdempotencyRecord and IdempotencyAcquireRequest.	Add helper to validate non-empty strings and a specific validator for the three idempotency key fields (operation, idempotency_key, body_hash). Add helper enforcing that COMPLETED idempotency records have a serialised_outcome. Replace duplicated inline validation in IdempotencyRecord.post_init and IdempotencyAcquireRequest.post_init with calls to these helpers.	episodic/canonical/idempotency.py
Factor upload size and string-field validation into shared helpers reused by Upload and UploadInitRequest.	Introduce helpers for non-negative integer size fields, non-empty strings, and non-negative optional actual_size. Update Upload.post_init and UploadInitRequest.post_init to use the new helpers instead of embedded validation logic, keeping behavior and messages consistent.	episodic/canonical/uploads.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 3 issues, and left some high level feedback:

• In ingestion_sources.py, the _require_exclusive_source docstring doesn’t match the implementation logic (it currently says 'when exactly one source identifier is not populated' but the check is for both populated or both empty); consider rewording to describe the invariant more accurately (e.g., 'unless exactly one of upload_id or source_uri is populated').
• Both idempotency.py and uploads.py define a _require_non_empty_string helper with identical semantics; if this duplication is intentional for domain isolation that’s fine, but otherwise consider a shared internal utility to avoid divergence in future changes.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `ingestion_sources.py`, the `_require_exclusive_source` docstring doesn’t match the implementation logic (it currently says 'when exactly one source identifier is not populated' but the check is for both populated or both empty); consider rewording to describe the invariant more accurately (e.g., 'unless exactly one of upload_id or source_uri is populated').
- Both `idempotency.py` and `uploads.py` define a `_require_non_empty_string` helper with identical semantics; if this duplication is intentional for domain isolation that’s fine, but otherwise consider a shared internal utility to avoid divergence in future changes.

## Individual Comments

### Comment 1
<location path="episodic/canonical/ingestion_sources.py" line_range="21" />
<code_context>
     SOURCE_URI = "source_uri"


+def _require_exclusive_source(has_upload: bool, has_uri: bool) -> None:
+    """Raise ValueError when exactly one source identifier is not populated."""
+    if has_upload == has_uri:
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining or consolidating these tiny validation helpers into one or two class-scoped methods so the dataclass’s control flow stays local and readable.

You’ve increased indirection without reducing complexity: five one-line module-level helpers for a single dataclass make control flow harder to follow and fragment the design.

You can keep all the validation and error messages exactly as-is while simplifying by:

1. Moving validation back into the dataclass (where the semantics clearly belong).
2. Grouping related checks into a single private method (or two at most), instead of five tiny helpers.
3. If you want to keep the logic reusable/testable, use a single `_validate` method or `@staticmethod` on the class.

For example:

```python
@dc.dataclass(frozen=True, slots=True)
class IngestionJobSource:
    # ... fields ...

    def __post_init__(self) -> None:
        """Validate attachment shape and weighting invariants."""
        self._validate()

    def _validate(self) -> None:
        has_upload = self.upload_id is not None
        has_uri = self.source_uri is not None

        if has_upload == has_uri:
            msg = "Exactly one of upload_id or source_uri must be populated."
            raise ValueError(msg)

        if self.attachment_kind is AttachmentKind.UPLOAD and not has_upload:
            msg = "upload attachments must populate upload_id."
            raise ValueError(msg)

        if self.attachment_kind is AttachmentKind.SOURCE_URI and not has_uri:
            msg = "source_uri attachments must populate source_uri."
            raise ValueError(msg)

        if not self.source_type.strip():
            msg = "source_type must be a non-empty string."
            raise ValueError(msg)

        if not 0 <= self.weight <= 1:
            msg = "weight must be between 0 and 1."
            raise ValueError(msg)
```

If you still prefer some decomposition, keep it coarse-grained and class-scoped, not five separate module helpers, e.g.:

```python
def _validate_sources(self, has_upload: bool, has_uri: bool) -> None:
    if has_upload == has_uri:
        msg = "Exactly one of upload_id or source_uri must be populated."
        raise ValueError(msg)
    if self.attachment_kind is AttachmentKind.UPLOAD and not has_upload:
        msg = "upload attachments must populate upload_id."
        raise ValueError(msg)
    if self.attachment_kind is AttachmentKind.SOURCE_URI and not has_uri:
        msg = "source_uri attachments must populate source_uri."
        raise ValueError(msg)
```

Then call this once from `_validate`. This keeps functionality identical but reduces surface area and improves readability.
</issue_to_address>

### Comment 2
<location path="episodic/canonical/idempotency.py" line_range="19" />
<code_context>
     COMPLETED = "completed"


+def _require_non_empty_string(value: str, field_name: str) -> None:
+    """Raise ValueError when *value* is blank after stripping whitespace."""
+    if not value.strip():
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the generic string validator and shared `_validate_idempotency_key_fields` helper with small field-specific helpers wired directly in each `__post_init__` to make the invariants clearer in place.

You can reduce the indirection while keeping the refactor’s DRY benefits by dropping the generic `_require_non_empty_string` and the “bag of fields” helper, and instead adding small, field-specific helpers.

This keeps messages and logic centralized, but makes each class’s invariants visible directly in `__post_init__` without stringly-typed field names.

```python
def _require_non_empty_operation(operation: str) -> None:
    if not operation.strip():
        raise ValueError("operation must be a non-empty string.")


def _require_non_empty_idempotency_key(idempotency_key: str) -> None:
    if not idempotency_key.strip():
        raise ValueError("idempotency_key must be a non-empty string.")


def _require_non_empty_body_hash(body_hash: str) -> None:
    if not body_hash.strip():
        raise ValueError("body_hash must be a non-empty string.")
```

Then wire them directly in each dataclass, keeping `_require_completed_has_outcome`:

```python
@dataclass(frozen=True, slots=True)
class IdempotencyRecord:
    ...
    def __post_init__(self) -> None:
        _require_non_empty_operation(self.operation)
        _require_non_empty_idempotency_key(self.idempotency_key)
        _require_non_empty_body_hash(self.body_hash)
        _require_completed_has_outcome(self.state, self.serialised_outcome)


@dataclass(frozen=True, slots=True)
class IdempotencyAcquireRequest:
    ...
    def __post_init__(self) -> None:
        _require_non_empty_operation(self.operation)
        _require_non_empty_idempotency_key(self.idempotency_key)
        _require_non_empty_body_hash(self.body_hash)
```

This removes the extra layer `_validate_idempotency_key_fields` and avoids passing field names around as strings, making the invariants easier to read in place while preserving the shared validation logic and behavior.
</issue_to_address>

### Comment 3
<location path="episodic/canonical/uploads.py" line_range="23" />
<code_context>
     EXPIRED = "expired"


+def _require_non_negative_size(value: int, field_name: str) -> None:
+    """Raise ValueError when an integer size field is negative."""
+    if value < 0:
</code_context>
<issue_to_address>
**issue (complexity):** Consider keeping validation logic per-class (with a single _validate method each) instead of multiple tiny module-level helper functions that add indirection without simplifying the code.

The new helpers add indirection without really simplifying the calling sites. Each function wraps a single if‑statement and message, so readers now have to jump between multiple tiny helpers to understand invariants that used to be local and domain-specific.

You can keep the validations cohesive and DRY without scattering them by grouping them into per-class helpers instead of multiple module-level utilities. For example:

```python
@dc.dataclass(frozen=True, slots=True)
class Upload:
    ...

    def __post_init__(self) -> None:
        """Validate upload invariants at the domain boundary."""
        self._validate()

    def _validate(self) -> None:
        if self.declared_size < 0:
            msg = "declared_size must be non-negative."
            raise ValueError(msg)

        if self.actual_size is not None and self.actual_size < 0:
            msg = "actual_size must be non-negative when provided."
            raise ValueError(msg)

        if not self.content_type.strip():
            msg = "content_type must be a non-empty string."
            raise ValueError(msg)

        if not self.storage_key.strip():
            msg = "storage_key must be a non-empty string."
            raise ValueError(msg)
```

And for `UploadInitRequest`:

```python
@dc.dataclass(frozen=True, slots=True)
class UploadInitRequest:
    ...

    def __post_init__(self) -> None:
        """Validate the client-declared upload metadata."""
        self._validate()

    def _validate(self) -> None:
        if self.declared_size < 0:
            msg = "declared_size must be non-negative."
            raise ValueError(msg)

        if not self.content_type.strip():
            msg = "content_type must be a non-empty string."
            raise ValueError(msg)
```

This keeps all checks close to their domain objects, avoids extra control-flow hops, and still gives you a single place per class to maintain the invariants. If you still want some shared logic, you can extract only the genuinely reusable pieces (e.g., a single `_require_non_negative_size`) and leave the rest inline to keep the code readable.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (complexity): Consider inlining or consolidating these tiny validation helpers into one or two class-scoped methods so the dataclass’s control flow stays local and readable.

You’ve increased indirection without reducing complexity: five one-line module-level helpers for a single dataclass make control flow harder to follow and fragment the design.

You can keep all the validation and error messages exactly as-is while simplifying by:

1. Moving validation back into the dataclass (where the semantics clearly belong).
2. Grouping related checks into a single private method (or two at most), instead of five tiny helpers.
3. If you want to keep the logic reusable/testable, use a single _validate method or @staticmethod on the class.

For example:

@dc.dataclass(frozen=True, slots=True)
class IngestionJobSource:
    # ... fields ...

    def __post_init__(self) -> None:
        """Validate attachment shape and weighting invariants."""
        self._validate()

    def _validate(self) -> None:
        has_upload = self.upload_id is not None
        has_uri = self.source_uri is not None

        if has_upload == has_uri:
            msg = "Exactly one of upload_id or source_uri must be populated."
            raise ValueError(msg)

        if self.attachment_kind is AttachmentKind.UPLOAD and not has_upload:
            msg = "upload attachments must populate upload_id."
            raise ValueError(msg)

        if self.attachment_kind is AttachmentKind.SOURCE_URI and not has_uri:
            msg = "source_uri attachments must populate source_uri."
            raise ValueError(msg)

        if not self.source_type.strip():
            msg = "source_type must be a non-empty string."
            raise ValueError(msg)

        if not 0 <= self.weight <= 1:
            msg = "weight must be between 0 and 1."
            raise ValueError(msg)

If you still prefer some decomposition, keep it coarse-grained and class-scoped, not five separate module helpers, e.g.:

def _validate_sources(self, has_upload: bool, has_uri: bool) -> None:
    if has_upload == has_uri:
        msg = "Exactly one of upload_id or source_uri must be populated."
        raise ValueError(msg)
    if self.attachment_kind is AttachmentKind.UPLOAD and not has_upload:
        msg = "upload attachments must populate upload_id."
        raise ValueError(msg)
    if self.attachment_kind is AttachmentKind.SOURCE_URI and not has_uri:
        msg = "source_uri attachments must populate source_uri."
        raise ValueError(msg)

Then call this once from _validate. This keeps functionality identical but reduces surface area and improves readability.

[sourcery-ai]

issue (complexity): Consider replacing the generic string validator and shared _validate_idempotency_key_fields helper with small field-specific helpers wired directly in each __post_init__ to make the invariants clearer in place.

You can reduce the indirection while keeping the refactor’s DRY benefits by dropping the generic _require_non_empty_string and the “bag of fields” helper, and instead adding small, field-specific helpers.

This keeps messages and logic centralized, but makes each class’s invariants visible directly in __post_init__ without stringly-typed field names.

def _require_non_empty_operation(operation: str) -> None:
    if not operation.strip():
        raise ValueError("operation must be a non-empty string.")


def _require_non_empty_idempotency_key(idempotency_key: str) -> None:
    if not idempotency_key.strip():
        raise ValueError("idempotency_key must be a non-empty string.")


def _require_non_empty_body_hash(body_hash: str) -> None:
    if not body_hash.strip():
        raise ValueError("body_hash must be a non-empty string.")

Then wire them directly in each dataclass, keeping _require_completed_has_outcome:

@dataclass(frozen=True, slots=True)
class IdempotencyRecord:
    ...
    def __post_init__(self) -> None:
        _require_non_empty_operation(self.operation)
        _require_non_empty_idempotency_key(self.idempotency_key)
        _require_non_empty_body_hash(self.body_hash)
        _require_completed_has_outcome(self.state, self.serialised_outcome)


@dataclass(frozen=True, slots=True)
class IdempotencyAcquireRequest:
    ...
    def __post_init__(self) -> None:
        _require_non_empty_operation(self.operation)
        _require_non_empty_idempotency_key(self.idempotency_key)
        _require_non_empty_body_hash(self.body_hash)

This removes the extra layer _validate_idempotency_key_fields and avoids passing field names around as strings, making the invariants easier to read in place while preserving the shared validation logic and behavior.

[sourcery-ai]

issue (complexity): Consider keeping validation logic per-class (with a single _validate method each) instead of multiple tiny module-level helper functions that add indirection without simplifying the code.

The new helpers add indirection without really simplifying the calling sites. Each function wraps a single if‑statement and message, so readers now have to jump between multiple tiny helpers to understand invariants that used to be local and domain-specific.

You can keep the validations cohesive and DRY without scattering them by grouping them into per-class helpers instead of multiple module-level utilities. For example:

@dc.dataclass(frozen=True, slots=True)
class Upload:
    ...

    def __post_init__(self) -> None:
        """Validate upload invariants at the domain boundary."""
        self._validate()

    def _validate(self) -> None:
        if self.declared_size < 0:
            msg = "declared_size must be non-negative."
            raise ValueError(msg)

        if self.actual_size is not None and self.actual_size < 0:
            msg = "actual_size must be non-negative when provided."
            raise ValueError(msg)

        if not self.content_type.strip():
            msg = "content_type must be a non-empty string."
            raise ValueError(msg)

        if not self.storage_key.strip():
            msg = "storage_key must be a non-empty string."
            raise ValueError(msg)

And for UploadInitRequest:

@dc.dataclass(frozen=True, slots=True)
class UploadInitRequest:
    ...

    def __post_init__(self) -> None:
        """Validate the client-declared upload metadata."""
        self._validate()

    def _validate(self) -> None:
        if self.declared_size < 0:
            msg = "declared_size must be non-negative."
            raise ValueError(msg)

        if not self.content_type.strip():
            msg = "content_type must be a non-empty string."
            raise ValueError(msg)

This keeps all checks close to their domain objects, avoids extra control-flow hops, and still gives you a single place per class to maintain the invariants. If you still want some shared logic, you can extract only the genuinely reusable pieces (e.g., a single _require_non_negative_size) and leave the rest inline to keep the code readable.