Plan REST surface hardening (4.1.2) - #111
Conversation
SummaryThis pull request implements roadmap item 4.1.2 ("Finalize REST surfaces for previous phase artefacts"), marking the work complete via a new ExecPlan document ( 1. Unified Error EnvelopeA new error serialisation layer ( 2. Pagination Envelopes &
|
| Layer / File(s) | Summary |
|---|---|
Local development environment ignore patterns .gitignore |
Add .claude/ to .gitignore and remove obsolete .repo.lock ignore entry. |
Roadmap and ExecPlan docs/execplans/4-1-2-finalize-rest-surfaces.md, docs/roadmap.md, docs/developers-guide.md, docs/users-guide.md, docs/episodic-podcast-generation-system-design.md |
Add ExecPlan for 4.1.2, mark task complete, and update user/developer/system-design docs to describe pagination envelopes, unified error contract ({code,message,details}), filter validation, and /v1 authorisation scaffold. |
API Infrastructure and Behaviour
| Layer / File(s) | Summary |
|---|---|
Central error envelope and mappers episodic/api/errors.py |
Add frozen ErrorEnvelope, serialize_http_error, http_error, validation_error, and domain-to-HTTP mappers (map_profile_template_error, map_reference_error) to produce canonical {code,message,details} payloads. |
Authorisation scaffold and middleware episodic/api/authorization.py, episodic/api/dependencies.py |
Introduce AuthorizationDecision, AuthorizationContext, AuthorizationPort protocol, PermitAll adapter, AuthorizationMiddleware (applies to /v1/ only), extend ApiDependencies.authorization with validation. |
Falcon app wiring episodic/api/app.py, episodic/api/__init__.py |
Wire AuthorizationMiddleware into app creation and register serialize_http_error as the app-wide Falcon error serialiser; re-export auth types from package root. |
Request parsing, handlers and resources
| Layer / File(s) | Summary |
|---|---|
Request helpers and validators episodic/api/helpers.py |
Replace direct Falcon exceptions with validation_error, add parse_pagination (returns Pagination), parse_optional_uuid_param, parse_enum_param, and stricter field/constraint metadata for validation errors. |
Handlers and history pagination episodic/api/handlers.py, episodic/api/resources/base.py |
Introduce HistoryRequest[EntityT], change handle_get_history to accept Pagination and expect (items, total) from services; centralise domain-error mapping via map_profile_template_error. |
Resource endpoints episodic/api/resources/* |
Update series-profiles, episode-templates, reference-documents, reference-bindings, resolved-bindings resources to parse pagination and filters, call paged service variants, and include items, limit, offset, and total in responses. |
Canonical services and storage
| Layer / File(s) | Summary |
|---|---|
Pagination value and services episodic/canonical/pagination.py, episodic/canonical/profile_templates/services/_generic.py, episodic/canonical/profile_templates/* |
Add Pagination type; add list_history_paged and list_entities_with_revisions_paged, extend kind-dispatch to support paged list/count helpers returning (items,total). |
Repository protocols and storage implementations episodic/canonical/*/types.py, episodic/canonical/reference_protocols.py, episodic/canonical/storage/* |
Add paged list and count methods to protocols; implement SQLAlchemy paged queries and explicit count queries; add _document_series_filter helper and reference repo count methods. |
Reference-domain helpers episodic/canonical/reference_documents/* |
Add list_reference_*_paged helpers that validate pagination, call storage list and count functions, and return (items,total); keep non-paged functions delegating to paged helpers. |
Tests and fixtures
| Layer / File(s) | Summary |
|---|---|
Test fixtures tests/fixtures/api.py |
Add build_api_dependencies to inject authorization test adapters; update fixtures to use it. |
Authorisation and envelope tests tests/test_api_authorization.py, tests/test_api_error_envelope.py |
Add tests for authorisation outcomes (200/401/403/503) and strict error-envelope assertions (code, message, details). |
Pagination and integration tests tests/*_pagination_*.py, tests/test_reference_document_roundtrip.py, updated tests |
Add parametrised tests for list/history pagination envelopes and totals; update existing tests to assert pagination fields and canonical error envelopes; add helpers asserting total >= returned items. |
Sequence Diagram(s)
(omit — changes are infra, contracts and multiple small interactions; no multi-component sequential diagram generated)
Possibly related PRs
- leynos/episodic#105: Related
/v1routing and middleware work that intersects with AuthorizationMiddleware and app wiring in this PR.
You are an AI agent. Produce a compact reviewer checklist for PR 111 summarising the major checkpoints: docs review, API wiring, auth middleware, error-envelope logic, pagination service changes, storage queries, and test updates.
Reviewer checklist (compact)
- Read ExecPlan and docs changes; confirm samples and roadmap marker.
- Verify .gitignore change only affects listed pattern.
- Inspect episodic/api/errors.py: validate ErrorEnvelope, serialize_http_error mapping, and domain->HTTP mappers.
- Inspect episodic/api/authorization.py and episodic/api/dependencies.py: validate AuthorizationPort API, PermitAll, middleware path gating, denial logging, and dependency validation.
- Inspect episodic/api/app.py and init.py: confirm middleware installed and serializer wired; confirm re-exports.
- Review episodic/api/helpers.py: confirm parse_pagination returns Pagination and validation_error usage for field/constraint metadata.
- Review episodic/api/handlers.py and resources/*: confirm handlers accept/propagate Pagination, HistoryRequest wiring, resource responses include limit/offset/total.
- Review episodic/canonical/pagination.py and profile_templates services: confirm list_history_paged and list_entities_with_revisions_paged signatures and behaviour.
- Review protocol/type changes: confirm new count/list_paged methods in reference and history protocols/types.
- Review SQLAlchemy storage changes: confirm paged queries, count implementations, and _document_series_filter correctness.
- Run or inspect tests/fixtures changes: confirm build_api_dependencies injection, canonical_api_creators fixture.
- Run test suite (or inspect tests): ensure new tests for auth, error envelopes, and pagination pass and updated assertions match envelope shape and total semantics.
- Spot-check logging and exception handling paths: adapter exception -> 503 envelope; unexpected decision -> 503 internal_error envelope.
- Confirm no exported public API regressions beyond documented new exports.
- Confirm pyproject.toml and lint config changes are intentional.
"Ignore the noise, make contracts bright,
Errors shaped and pages sliced just right,
Authorise /v1 with a scaffold light,
Tests assert totals, docs mark the flight."
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
4-1-2-finalize-rest-surfaces
Reviewer's GuideAdds a comprehensive execution plan document for roadmap item 4.1.2 that specifies how to harden existing /v1 REST endpoints with a unified error envelope, consistent pagination (including total counts), filter handling, and an authorization scaffold, without changing any production behavior yet. Sequence diagram for authorization scaffold and unified error envelopesequenceDiagram
actor Client
participant FalconApp
participant AuthorizationMiddleware
participant Resource
participant AuthorizationPort
participant ErrorHandler as handle_http_error
Client->>FalconApp: HTTP request /v1/series-profiles
FalconApp->>AuthorizationMiddleware: process_request(req, resp)
AuthorizationMiddleware->>AuthorizationPort: decide(AuthorizationContext)
AuthorizationPort-->>AuthorizationMiddleware: AuthorizationDecision.permit
AuthorizationMiddleware-->>FalconApp: continue
FalconApp->>Resource: on_get(req, resp)
alt [domain or validation error]
Resource->>FalconApp: raise falcon.HTTPError
FalconApp->>ErrorHandler: handle_http_error(req, resp, exc, params)
ErrorHandler-->>Client: 4xx/5xx ErrorEnvelope
else [success]
Resource-->>Client: 2xx response (possibly pagination envelope)
end
Flow diagram for paginated list endpoints with total countsflowchart LR
Client[Client] --> ApiResource[API Resource on_get]
ApiResource --> Helpers[parse_pagination]
Helpers --> Service[list_reference_documents_paged]
Service --> RepoList[ReferenceDocumentRepository.list_for_series]
Service --> RepoCount[ReferenceDocumentRepository.count_for_series]
RepoList --> DB[(Database)]
RepoCount --> DB
Service --> ApiResource
ApiResource --> Response["Pagination envelope {items, limit, offset, total}"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
bb5ef38 to
348b380
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/execplans/4-1-2-finalize-rest-surfaces.md`:
- Line 149: The document uses mixed -ise/-yse spellings; normalize to
en-GB-oxendict forms using -ize and -lyse (e.g., replace "stabilise" with
"stabilize", "centralised" → "centralized", "serialisation" → "serialization",
"organisations" → "organizations", "optimisation" → "optimization", "organised"
→ "organized" and any -lyse variants to -lyse), and apply the same replacements
in the other flagged locations (lines referenced: 230, 338, 347, 357, 549) so
the whole file consistently follows the mandated -ize/-lyse suffix convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 493a352b-0423-434b-b3dd-29f9280efd76
📒 Files selected for processing (2)
.gitignoredocs/execplans/4-1-2-finalize-rest-surfaces.md
Draft the pre-implementation ExecPlan for roadmap item `4.1.2` at
`docs/execplans/4-1-2-finalize-rest-surfaces.md`. The plan covers eight
milestones that finalize REST surfaces for previously implemented `/v1`
canonical resources: a unified `{code, message, details}` error envelope
served through a single Falcon `add_error_handler`, the
`{items, limit, offset, total}` pagination envelope across every list
endpoint, consistent filter parameter parsing, and an
inbound-adapter-local `AuthorizationPort` scaffold with a permit-all
default. Full Role-Based Access Control (RBAC) and tenancy isolation
remain roadmap item `5.1`.
Decisions are recorded in the plan's `Decision log`: additive `count_*`
Protocol methods (so existing list signatures stay backwards-compatible),
Pattern B (centralised Falcon error handler) plus per-family
classification helpers, scaffold port placed at
`episodic/api/authorization.py`, resolved-bindings paginated at the API
layer, and routine readiness `503` responses exempt from the envelope
rewrite. References to `docs/episodic-tui-api-design.md`,
`docs/episodic-podcast-generation-system-design.md`, and ADRs 002, 009,
and 014 anchor the constraints. Three Wyvern research subagents
contributed the API surface inventory, the test coverage map, and the
hexagonal/architecture rails check that underpin the plan.
Implementation is gated on user approval.
The Claude Code harness writes a per-session lock file at `.claude/scheduled_tasks.lock` (sessionId, pid, procStart, acquiredAt). This is local tooling state, not source, and mirrors the existing treatment of `.agents/mcp/context_pack/packs/.repo.lock` already ignored at line 10. Drop the duplicated `.agents/...` entry while adding `.claude/`.
Update `docs/execplans/4-1-2-finalize-rest-surfaces.md` after the rebase
onto `origin/main` picked up commit `3403ace` ("Adopt Hecate for
architecture checks (#107)"), which removed the repo-local
`episodic.architecture` module and moved policy enforcement into
`[tool.hecate]` in `pyproject.toml:437-496`.
- Replace `episodic/architecture/policy.py:<line>` citations with
`pyproject.toml:<line>` citations covering the same group-prefix and
allowed-import rules.
- Replace the `python -m episodic.architecture` recovery instruction
with `uv run hecate check`, noting that diagnostic identifier
`ARCH001` carries over.
- Append a revision note documenting the rebase and confirming the
policy semantics (group prefixes, allowed imports) are
byte-equivalent to the previous Python-module implementation, so no
work-plan milestone change was required.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
episodic/canonical/profile_templates/services/_generic.py (1)
65-181: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit
_get_repos_for_kindinto per-kind builders and shrink the method.Reduce the branching and nested local function load in Line 65 onward by extracting each kind into dedicated helper factories, then keep
_get_repos_for_kindas a thin dispatcher. The current shape already trips the pipeline’s large-method gate and will keep degrading maintainability as new kinds are added.As per coding guidelines "Keep C90 / mccabe complexity ≤ 9".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/profile_templates/services/_generic.py` around lines 65 - 181, The function _get_repos_for_kind is too large and branches per EntityKind (EntityKind.SERIES_PROFILE / "series_profile" and EntityKind.EPISODE_TEMPLATE / "episode_template") with many nested local async functions; extract each branch into its own helper factory (e.g., _build_series_profile_kind(uow: CanonicalUnitOfWork) -> _KindDispatch and _build_episode_template_kind(uow: CanonicalUnitOfWork) -> _KindDispatch) that move the repository casts, the nested async functions (_list_profiles, _count_profiles, _list_profile_history_paged, _list_templates, _list_template_history_paged) and the _KindDispatch construction into the helper; then make _get_repos_for_kind a thin dispatcher that calls the appropriate helper based on kind and returns its _KindDispatch. Ensure signatures and returned callable types (entity_get, fetch_latest, list_history_for_parent, list_history_for_parent_paged, count_history_for_parent, list_entities, count_entities, get_latest_revisions) match the originals.tests/test_api_error_envelope.py (1)
46-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParametrize the duplicated validation/envelope tests
Remove the repeated arrange/act/assert bodies and replace them with a single
@pytest.mark.parametrizematrix covering all 7 cases intests/test_api_error_envelope.py.Suggested refactor shape
+@pytest.mark.parametrize( + ("path", "params", "status_code", "code", "field", "constraint"), + [ + ( + "/v1/series-profiles/not-a-valid-uuid", + None, + 400, + "validation_error", + "profile_id", + "uuid", + ), + ( + "/v1/reference-bindings", + {"target_kind": "episode_template", "target_id": "irrelevant", "limit": "0"}, + 400, + "validation_error", + "limit", + "range", + ), + ( + "/v1/episode-templates", + {"series_profile_id": "not-a-uuid"}, + 400, + "validation_error", + "series_profile_id", + "uuid", + ), + ( + "/v1/series-profiles/018f0c2a-1234-7000-a000-000000000001/reference-documents", + {"kind": "not-a-kind"}, + 400, + "validation_error", + "kind", + "enum", + ), + ( + "/v1/reference-bindings", + {"target_kind": "not-a-target", "target_id": "018f0c2a-1234-7000-a000-000000000001"}, + 400, + "validation_error", + "target_kind", + "enum", + ), + ( + "/v1/series-profiles/018f0c2a-1234-7000-a000-000000000001/resolved-bindings", + None, + 400, + "validation_error", + "episode_id", + "required", + ), + ( + "/v1/series-profiles/018f0c2a-1234-7000-a000-000000000001", + None, + 404, + "not_found", + None, + None, + ), + ], +) +def test_error_envelope_matrix( + canonical_api_client: testing.TestClient, + path: str, + params: dict[str, str] | None, + status_code: int, + code: str, + field: str | None, + constraint: str | None, +) -> None: + simulate_get_kwargs: dict[str, object] = {} + if params is not None: + simulate_get_kwargs["params"] = params + + response = canonical_api_client.simulate_get(path, **simulate_get_kwargs) + + _assert_error_envelope( + response, + status_code=status_code, + code=code, + field=field, + constraint=constraint, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_api_error_envelope.py` around lines 46 - 166, Multiple tests in tests/test_api_error_envelope.py repeat the same arrange/act/assert pattern; consolidate them by replacing the seven individual test_... functions with a single parametrized test using pytest.mark.parametrize that iterates over cases (use identifiers and expected attrs for: test_invalid_uuid_returns_validation_envelope, test_invalid_pagination_returns_validation_envelope, test_invalid_optional_uuid_filter_returns_validation_envelope, test_invalid_reference_document_kind_filter_returns_validation_envelope, test_invalid_reference_binding_target_kind_returns_validation_envelope, test_missing_query_parameter_returns_validation_envelope, test_unknown_identifier_returns_not_found_envelope), call canonical_api_client.simulate_get per case, and assert via _assert_error_envelope using the tuple fields (path, params, expected status_code, code, optional field and constraint) so each scenario is covered without duplicated bodies.episodic/api/handlers.py (1)
150-224: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSplit
handle_update_entityinto focused helpers to clear the quality gate.Extract required-field validation and error remapping from this method. Keep the method as request orchestration only so the CodeScene large-method gate stops failing.
♻️ Proposed refactor
+def _validate_required_fields( + payload: JsonPayload, + required_fields: tuple[str, ...], +) -> None: + for field_name in required_fields: + if field_name not in payload: + msg = f"Missing required field: {field_name}" + raise validation_error(msg, field=field_name, constraint="required") + + +def _raise_mapped_update_error( + exc: EntityNotFoundError | RevisionConflictError, + *, + parsed_entity_id: uuid.UUID, + payload: JsonPayload, +) -> None: + if isinstance(exc, EntityNotFoundError): + raise map_profile_template_error(exc, entity_id=parsed_entity_id) from exc + expected_revision = typ.cast("int | None", payload.get("expected_revision")) + raise map_profile_template_error( + exc, + entity_id=parsed_entity_id, + expected_revision=expected_revision, + ) from exc @@ - for field_name in required_fields: - if field_name not in payload: - msg = f"Missing required field: {field_name}" - raise validation_error(msg, field=field_name, constraint="required") + _validate_required_fields(payload, required_fields) @@ - except EntityNotFoundError as exc: - raise map_profile_template_error(exc, entity_id=parsed_entity_id) from exc - except RevisionConflictError as exc: - expected_revision = typ.cast("int | None", payload.get("expected_revision")) - raise map_profile_template_error( - exc, - entity_id=parsed_entity_id, - expected_revision=expected_revision, - ) from exc + except (EntityNotFoundError, RevisionConflictError) as exc: + _raise_mapped_update_error( + exc, + parsed_entity_id=parsed_entity_id, + payload=payload, + )As per coding guidelines, "
**/*.py: Keep C90 / mccabe complexity ≤ 9".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/api/handlers.py` around lines 150 - 224, handle_update_entity is too large/complex; extract the required-field validation loop and the exception remapping into two small helpers so handle_update_entity becomes orchestration-only. Create validate_required_fields(payload, required_fields) that raises validation_error with the same message/params when a field is missing, and create remap_update_exceptions(fn) or remap_update_errors(exc, parsed_entity_id, payload) which contains the EntityNotFoundError and RevisionConflictError handling currently in the try/except (use the same calls to map_profile_template_error and preserve expected_revision extraction). Replace the inline loop and except blocks in handle_update_entity with calls to validate_required_fields(...) and the new remapping helper, leaving request_builder(...), the uow/service call, and serializer_fn(...) unchanged.episodic/api/helpers.py (1)
118-124: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCollapse pagination parsing into one field-aware helper.
Parse
limitandoffsetonce each and raise from that helper directly. The
current flow reparses both values inside_pagination_type_error_field(), which
is the duplication tripping the CodeScene gate and is easy to desynchronise
later.♻️ Proposed refactor
+def _parse_int_query_param( + raw_value: str | None, + *, + name: str, + default: int, +) -> int: + """Parse an optional integer query parameter by name.""" + if raw_value is None: + return default + try: + return int(raw_value) + except ValueError as exc: + msg = f"{name} must be an integer." + raise validation_error(msg, field=name, constraint="type") from exc + + def parse_pagination(req: falcon.Request) -> tuple[int, int]: """Parse and validate common `limit`/`offset` query parameters.""" - raw_limit = req.get_param("limit") - raw_offset = req.get_param("offset") - - try: - limit = _DEFAULT_PAGE_LIMIT if raw_limit is None else int(raw_limit) - offset = 0 if raw_offset is None else int(raw_offset) - except ValueError as exc: - msg = "Pagination parameters limit/offset must be integers." - field = _pagination_type_error_field(raw_limit, raw_offset) - raise validation_error(msg, field=field, constraint="type") from exc + limit = _parse_int_query_param( + req.get_param("limit"), + name="limit", + default=_DEFAULT_PAGE_LIMIT, + ) + offset = _parse_int_query_param( + req.get_param("offset"), + name="offset", + default=0, + ) if limit < 1 or limit > _MAX_PAGE_LIMIT: msg = f"limit must be between 1 and {_MAX_PAGE_LIMIT}." raise validation_error(msg, field="limit", constraint="range") if offset < 0: msg = "offset must be a non-negative integer." raise validation_error(msg, field="offset", constraint="range") return limit, offset - - -def _pagination_type_error_field( - raw_limit: str | None, - raw_offset: str | None, -) -> str: - """Return the first pagination field that failed integer parsing.""" - if raw_limit is not None: - try: - int(raw_limit) - except ValueError: - return "limit" - if raw_offset is not None: - try: - int(raw_offset) - except ValueError: - return "offset" - return "limit"Also applies to: 159-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/api/helpers.py` around lines 118 - 124, Replace the current ad-hoc parsing in parse_pagination with a small helper _parse_int_query_param(raw_value, *, name, default) that returns default when raw_value is None, tries int(raw_value) and on ValueError raises validation_error(f"{name} must be an integer.", field=name, constraint="type"); then call this helper for "limit" (default _DEFAULT_PAGE_LIMIT) and "offset" (default 0) inside parse_pagination, remove _pagination_type_error_field, and keep the existing range checks for limit and offset unchanged (refer to functions _parse_int_query_param, parse_pagination, and remove _pagination_type_error_field).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Line 109: Change the spelling in the REST error contract sentence that
currently reads "Every Falcon `HTTPError` raised by the canonical API is
serialised as" to use en-GB-oxendict - replace "serialised" with "serialized" so
the line becomes "Every Falcon `HTTPError` raised by the canonical API is
serialized as".
In `@episodic/api/authorization.py`:
- Around line 88-109: Replace the typing.assert_never(decision) call in the
match on AuthorizationDecision with an explicit defensive fallback that logs the
unexpected decision and returns a canonical JSON 503 response: call
_log_authorization_denial(decision, context) (or a dedicated logger) and set
resp.media to a structured error (e.g., code "internal_error", message
"Unexpected authorization decision", details {} or minimal context), set
resp.status = falcon.HTTP_503 and resp.complete = True; remove
typ.assert_never(decision) so an unexpected enum value cannot escape as an
unstructured 500.
In `@episodic/api/errors.py`:
- Around line 335-338: The except clause in the _status_code function uses
Python 2 syntax (`except IndexError, ValueError:`) which is invalid in Python 3;
change it to use a tuple of exceptions (`except (IndexError, ValueError):`) so
the block that returns 500 on parse errors executes correctly when
int(exc.status.split(...)[0]) fails.
In `@episodic/canonical/profile_templates/services/_generic.py`:
- Line 259: The inline lint suppression on the function definition
list_history_paged currently uses a bare "# noqa: PLR0913"; update it to include
a short justification (or remove it if you can refactor to reduce parameters) —
e.g. append " - justification: [brief reason]" to the noqa so it reads like the
other suppression style in this file; ensure the justification explains why the
parameter-count complexity is acceptable for list_history_paged.
In `@episodic/canonical/storage/repositories.py`:
- Around line 118-124: The paginated query orders only by
SeriesProfileRecord.created_at which can tie and cause page drift; update the
query that builds the statement (the variable named statement in the
SeriesProfileRecord select) to add a deterministic tie-breaker to the ORDER BY,
e.g. append SeriesProfileRecord.id (or the record's primary key/unique column)
after created_at so results are fully deterministic; apply the same change to
the other similar query in this file (the select around the block at the later
occurrence ordering by created_at).
In `@tests/api_fixtures.py`:
- Around line 191-195: The test currently asserts exact equality between
payload["total"] and len(items) using the _assert_equal call, which fails for
paginated responses; change the assertion to verify payload["total"] is >=
len(items) (i.e., replace the equality check with a greater-or-equal comparison)
so total represents the full filtered count while len(items) is the returned
page size; apply the same replacement for the second occurrence of the
_assert_equal check later in the file (the one at the other noted block).
In `@tests/test_api_authorization.py`:
- Around line 74-137: Collapse the three duplicate tests
(test_deny_all_authorization_returns_unauthorized_envelope,
test_forbidden_authorization_returns_forbidden_envelope,
test_authorization_adapter_exception_returns_503) into a single parametrized
pytest function that iterates over (authorization_adapter, expected_status,
expected_payload) tuples; for each case call _build_client(session_factory,
<adapter>), client.simulate_get("/v1/series-profiles"), assert
response.status_code == expected_status, and assert response.json ==
expected_payload, referencing the existing adapters DenyAllAuthorization,
ForbidSeriesProfilesAuthorization, and RaisingAuthorization to locate
implementations.
In `@tests/test_binding_resolution_api.py`:
- Around line 77-81: The new bare asserts in test_binding_resolution_api.py
should include explicit failure messages; update the assertions on
payload["limit"], payload["offset"], payload["total"] and the items-related
asserts (and the other two asserts around lines 119-120) to use assert
<condition>, "<descriptive message>" and include the actual/expected values in
the message for easier debugging (e.g., assert payload["limit"] == 20,
f"expected limit == 20, got {payload['limit']}"); locate these assertions by the
symbols payload and items in the test_binding_resolution_api.py test function
and replace each bare assert with one that supplies a clear, specific message.
In `@tests/test_reference_document_roundtrip.py`:
- Around line 60-107: Replace the bare asserts in the test (all assertions
referencing first_page_response, first_page["items"], first_page["total"],
revision_page_response, revision_page["items"], revision_page["total"],
binding_page_response, binding_page["items"], and binding_page["total"]) with
message-bearing asserts (use assert <condition>, "<descriptive message>") so
failures show deterministic context; update the checks around
support.create_reference_document_revision, simulate_get responses, and casting
validations to include clear messages like "expected status 200 for <response
variable>", "expected X items in <page variable>", or "expected total Y for
<page variable>" to comply with the assertion style guideline.
---
Outside diff comments:
In `@episodic/api/handlers.py`:
- Around line 150-224: handle_update_entity is too large/complex; extract the
required-field validation loop and the exception remapping into two small
helpers so handle_update_entity becomes orchestration-only. Create
validate_required_fields(payload, required_fields) that raises validation_error
with the same message/params when a field is missing, and create
remap_update_exceptions(fn) or remap_update_errors(exc, parsed_entity_id,
payload) which contains the EntityNotFoundError and RevisionConflictError
handling currently in the try/except (use the same calls to
map_profile_template_error and preserve expected_revision extraction). Replace
the inline loop and except blocks in handle_update_entity with calls to
validate_required_fields(...) and the new remapping helper, leaving
request_builder(...), the uow/service call, and serializer_fn(...) unchanged.
In `@episodic/api/helpers.py`:
- Around line 118-124: Replace the current ad-hoc parsing in parse_pagination
with a small helper _parse_int_query_param(raw_value, *, name, default) that
returns default when raw_value is None, tries int(raw_value) and on ValueError
raises validation_error(f"{name} must be an integer.", field=name,
constraint="type"); then call this helper for "limit" (default
_DEFAULT_PAGE_LIMIT) and "offset" (default 0) inside parse_pagination, remove
_pagination_type_error_field, and keep the existing range checks for limit and
offset unchanged (refer to functions _parse_int_query_param, parse_pagination,
and remove _pagination_type_error_field).
In `@episodic/canonical/profile_templates/services/_generic.py`:
- Around line 65-181: The function _get_repos_for_kind is too large and branches
per EntityKind (EntityKind.SERIES_PROFILE / "series_profile" and
EntityKind.EPISODE_TEMPLATE / "episode_template") with many nested local async
functions; extract each branch into its own helper factory (e.g.,
_build_series_profile_kind(uow: CanonicalUnitOfWork) -> _KindDispatch and
_build_episode_template_kind(uow: CanonicalUnitOfWork) -> _KindDispatch) that
move the repository casts, the nested async functions (_list_profiles,
_count_profiles, _list_profile_history_paged, _list_templates,
_list_template_history_paged) and the _KindDispatch construction into the
helper; then make _get_repos_for_kind a thin dispatcher that calls the
appropriate helper based on kind and returns its _KindDispatch. Ensure
signatures and returned callable types (entity_get, fetch_latest,
list_history_for_parent, list_history_for_parent_paged,
count_history_for_parent, list_entities, count_entities, get_latest_revisions)
match the originals.
In `@tests/test_api_error_envelope.py`:
- Around line 46-166: Multiple tests in tests/test_api_error_envelope.py repeat
the same arrange/act/assert pattern; consolidate them by replacing the seven
individual test_... functions with a single parametrized test using
pytest.mark.parametrize that iterates over cases (use identifiers and expected
attrs for: test_invalid_uuid_returns_validation_envelope,
test_invalid_pagination_returns_validation_envelope,
test_invalid_optional_uuid_filter_returns_validation_envelope,
test_invalid_reference_document_kind_filter_returns_validation_envelope,
test_invalid_reference_binding_target_kind_returns_validation_envelope,
test_missing_query_parameter_returns_validation_envelope,
test_unknown_identifier_returns_not_found_envelope), call
canonical_api_client.simulate_get per case, and assert via
_assert_error_envelope using the tuple fields (path, params, expected
status_code, code, optional field and constraint) so each scenario is covered
without duplicated bodies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9df4dca3-7f70-40f8-9e7f-e31e0ef0eef8
📒 Files selected for processing (44)
docs/developers-guide.mddocs/episodic-podcast-generation-system-design.mddocs/execplans/4-1-2-finalize-rest-surfaces.mddocs/roadmap.mddocs/users-guide.mdepisodic/api/__init__.pyepisodic/api/app.pyepisodic/api/authorization.pyepisodic/api/dependencies.pyepisodic/api/errors.pyepisodic/api/handlers.pyepisodic/api/helpers.pyepisodic/api/resources/base.pyepisodic/api/resources/episode_templates.pyepisodic/api/resources/reference_bindings.pyepisodic/api/resources/reference_documents.pyepisodic/api/resources/resolved_bindings.pyepisodic/api/resources/series_profiles.pyepisodic/canonical/history_protocols.pyepisodic/canonical/profile_templates/__init__.pyepisodic/canonical/profile_templates/services/__init__.pyepisodic/canonical/profile_templates/services/_generic.pyepisodic/canonical/profile_templates/types.pyepisodic/canonical/reference_documents/__init__.pyepisodic/canonical/reference_documents/bindings.pyepisodic/canonical/reference_documents/documents.pyepisodic/canonical/reference_documents/revisions.pyepisodic/canonical/reference_protocols.pyepisodic/canonical/storage/history_repositories.pyepisodic/canonical/storage/reference_repositories.pyepisodic/canonical/storage/repositories.pytests/api_fixtures.pytests/fixtures/api.pytests/test_api_authorization.pytests/test_api_error_envelope.pytests/test_api_route_versioning.pytests/test_binding_resolution_api.pytests/test_http_service_scaffold.pytests/test_lifespan_hooks.pytests/test_profile_template_api.pytests/test_profile_template_history_pagination_api.pytests/test_profile_template_pagination_api.pytests/test_reference_document_api_support.pytests/test_reference_document_roundtrip.py
Introduce a Falcon ASGI error handler that serialises canonical API
HTTP errors as the documented `{code, message, details}` envelope.
Attach validation details in shared request parsers and preserve
profile/template optimistic-lock context for stale updates.
Update route-versioning, binding-resolution, profile/template, and
reference-document tests to assert the new envelope. Document the
contract in the developer guide and record Milestone 1 progress in the
active execplan.
Add repository count methods and paged service wrappers for reusable reference documents, document revisions, and bindings. Return `total` in all three existing reference-domain list envelopes without changing the legacy list service return types. Extend reference API tests to assert totals for single-item helpers and multi-page list responses.
Add paged profile/template repository and service reads so list
endpoints return `{items, limit, offset, total}`. Apply the same
envelope to resolved bindings and cover ordering in API tests.
Thread paged history reads through the profile/template repository, service, and shared Falcon history resource so history endpoints return the standard pagination envelope and validate limit/offset inputs.
Add shared optional UUID and enum query parsers, then use them across list resources so invalid filters fail before service dispatch with field-level validation error details.
Introduce an async authorization port, permit-all adapter, and Falcon middleware for /v1 requests. Cover unauthorized, forbidden, bypass, and adapter-failure envelopes while documenting the scaffold.
Document the standardized REST pagination and error envelopes, filter parsing behaviour, and permit-all authorization scaffold across the user guide, developer guide, and system design.
Update the canonical route smoke test to assert the final REST list contract after the 4.1.2 pagination work. Record the final gate findings in the ExecPlan, including the stale assertion fix and the pre-existing guest-bios property failure that remains outside this change.
Close out the 4.1.2 ExecPlan with final gate evidence, CodeRabbit review results, and the known out-of-scope guest-bios property failure. Mark roadmap item 4.1.2 complete after the final review returned zero findings.
Apply en-GB Oxford `-ize` spellings consistently in the 4.1.2 ExecPlan after review flagged mixed suffix forms.
The previous spelling-normalization pass missed four `-ise` variants in `docs/execplans/4-1-2-finalize-rest-surfaces.md`. Switch each to its Oxford `-ize` form per `en-gb-oxendict`: - `initialisation` → `initialization` - `organisation` → `organization` - `standardised` → `standardized` - `serialises` → `serializes` Greek-origin `-lyse` words (`analyse`, `paralyse`) are not used in the file and so remain unchanged; `surprise` (and its plural form `surprises` used in the section heading) is preserved because it is Old-French-derived and not subject to the `-ize` rule.
The rebase onto `origin/main` brought in main's commit `7e4631d`
("Consolidate metrics/clock ports and tighten checkpoint metrics
tests (#115) (#116)"), which independently added `.claude/` near the
top of `.gitignore`. The earlier branch-local commit
`a59c343` ("Ignore `.claude/` Claude Code harness state") had added the
same entry at the bottom of the file. Drop the now-redundant trailing
entry; main's placement is kept because it groups with the other
`.cache`-style entries.
8307d40 to
140ed87
Compare
`make markdownlint` failed with MD012 on `docs/users-guide.md:200` after the rebase onto `origin/main` left a stray double blank line between the closing reference-documents bullet and the `### REST API reference` heading. Drop the extra blank.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. episodic/canonical/profile_templates/services/_generic.py Comment on lines +81 to +135 def _build_series_profile_dispatch(uow: CanonicalUnitOfWork) -> _KindDispatch:
"""Return the dispatch table for series-profile repositories."""
profile_repo = typ.cast("_SeriesProfileRepository", uow.series_profiles)
profile_history_repo = typ.cast(
"_SeriesProfileHistoryRepository",
uow.series_profile_history,
)
async def _list_profiles(
_: uuid.UUID | None,
limit: int | None,
offset: int,
) -> cabc.Sequence[object]:
return typ.cast(
"cabc.Sequence[object]",
await profile_repo.list(limit=limit, offset=offset),
)
async def _count_profiles(_: uuid.UUID | None) -> int:
return await profile_repo.count()
async def _list_profile_history_paged(
profile_id: uuid.UUID,
limit: int,
offset: int,
) -> list[object]:
return typ.cast(
"list[object]",
await profile_history_repo.list_for_profile_paged(
profile_id,
limit=limit,
offset=offset,
),
)
return _KindDispatch(
human_label="Series profile",
entity_get=typ.cast(
"cabc.Callable[[uuid.UUID], cabc.Awaitable[object | None]]",
profile_repo.get,
),
fetch_latest=typ.cast(
"_RevisionFetcher",
profile_history_repo.get_latest_for_profile,
),
list_history_for_parent=typ.cast(
"cabc.Callable[[uuid.UUID], cabc.Awaitable[list[object]]]",
profile_history_repo.list_for_profile,
),
list_history_for_parent_paged=_list_profile_history_paged,
count_history_for_parent=profile_history_repo.count_for_profile,
list_entities=_list_profiles,
count_entities=_count_profiles,
get_latest_revisions=profile_history_repo.get_latest_revisions_for_profiles,
)❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationtests/test_api_error_envelope.py: What lead to degradation?The module contains 6 functions with similar structure: test_invalid_optional_uuid_filter_returns_validation_envelope,test_invalid_pagination_returns_validation_envelope,test_invalid_reference_binding_target_kind_returns_validation_envelope,test_invalid_reference_document_kind_filter_returns_validation_envelope and 2 more functions Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationepisodic/canonical/profile_templates/types.py: What lead to degradation?The module contains 2 functions with similar structure: init,count_for_template Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Large Methodtests/test_binding_resolution_api.py: test_resolved_bindings_endpoint_returns_resolved_payloads What lead to degradation?test_resolved_bindings_endpoint_returns_resolved_payloads has 79 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Python language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Large Methodtests/test_reference_document_roundtrip.py: test_reference_document_lists_report_total_across_pages What lead to degradation?test_reference_document_lists_report_total_across_pages has 86 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Python language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
`test_reference_document_lists_report_total_across_pages` exceeded the
70-line CodeScene threshold for a single function because it interleaved
setup, request, and assertion code for three distinct list endpoints
(reference documents, revisions, bindings). Extract one helper per stage:
- `_assert_document_page_total(client, profile_id) -> list[str]`
creates three reference documents and asserts the first page of
`/v1/series-profiles/{profile_id}/reference-documents`.
- `_assert_revision_page_total(client, profile_id, document_id) -> list[str]`
creates two revisions on the given document and asserts the first page of
`.../reference-documents/{document_id}/revisions`.
- `_assert_binding_page_total(client, revision_ids, template_id) -> None`
binds each revision to the template and asserts the first page of
`/v1/reference-bindings`.
The test function now reads top-to-bottom in 12 lines: build fixture →
document page → revision page → binding page. The sibling test
`test_reference_document_round_trip_and_binding_workflow` is untouched.
Behaviour is unchanged; both tests pass in ~5 s.
`test_resolved_bindings_endpoint_returns_resolved_payloads` ran roughly 79 lines because it interleaved three concerns inside one test body: creating a series-profile binding, creating an episode-template binding, and asserting the pagination envelope plus item content of the resolved-bindings response. Extract one helper per concern: - `_create_series_profile_binding(client, profile_id, episode_id) -> str` creates the style-guide document, one revision, and the series-profile binding, returning the revision id. - `_create_episode_template_binding(client, profile_id, template_id) -> str` creates the guest-profile document, one revision, and the episode-template binding, returning the revision id. - `_assert_resolved_bindings_payload(payload, series_revision_id, template_revision_id)` asserts the limit/offset/total envelope and the per-item revision and document content. The test now reads top-to-bottom in ~53 lines: build fixture, create episode, create the two bindings via the new helpers, request the resolved-bindings endpoint, and assert. Sibling tests in the file are untouched. Behaviour is unchanged; all 8 tests in the file still pass in ~17 s.
CodeScene flagged seven structurally identical test functions in `tests/test_api_error_envelope.py`. Each ran the same two-step body — `canonical_api_client.simulate_get(path, params=...)` followed by `_assert_error_envelope(response, _Expected(...))` — and varied only in the URL path, optional query params, and the `_Expected` envelope. Collapse them into a single `test_error_envelope` driven by `@pytest.mark.parametrize`. Each `pytest.param` carries the original inputs and an explicit `id=` matching the prior test's intent (`invalid_uuid_path_segment`, `invalid_pagination_bounds`, `invalid_optional_uuid_filter`, `invalid_reference_document_kind_filter`, `invalid_reference_binding_target_kind`, `missing_required_query_parameter`, `unknown_identifier`), so failing cases still report a descriptive identifier. The `_Expected` dataclass and `_assert_error_envelope` helper are untouched; only the import block grows by one line (`import pytest`). All seven parametrised cases pass. No external module imported any of the removed function names.
CodeScene flagged the two private helpers `_build_series_profile_dispatch` and `_build_episode_template_dispatch` as a code-duplication finding: they shared structure but diverged on the entity kind. Inline each builder's body directly into the corresponding match arm of `_get_repos_for_kind`, then delete both helpers. The closure names (`_list_entities`, `_count_entities`, `_list_history_paged`) are intentionally identical across the two arms; only one arm runs at runtime, so the shared names cause no collision. Two implementation notes: - The episode-template arm now wraps `template_repo.count` in an async closure that threads `series_profile_id` through to `template_repo.count(series_profile_id)`. The previous helper passed the bound method directly, which worked because `template_repo.count` already accepts the same parameter, but the explicit closure mirrors the series-profile arm and matches the task's target shape. - Per-arm history-repository casts use distinct names (`profile_history_repo`, `template_history_repo`) rather than a shared `history_repo`. Python's `match` does not introduce a fresh scope per arm, so a shared name would be unioned by static analysis and the per-arm closures could not resolve their kind-specific methods (`list_for_profile_paged` vs `list_for_template_paged`). The short comment at the top of the function records this for future readers. `_get_repos_for_kind` carries a single targeted suppression, `# noqa: C901`, with a justification tying the lift in cyclomatic complexity to this design choice (extraction was just removed at the reviewer's request). This is the rule's last-resort case: the design and the linter conflict, and the design has been explicitly chosen. Behaviour is unchanged. Full test suite: 782 passed, 3 skipped.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/test_binding_resolution_api.py (1)
19-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAddress the code duplication flagged by CodeScene.
Both
_create_series_profile_bindingand_create_episode_template_bindingfollow the same pattern: create a document with revision, create a binding, and return the revision ID. The pipeline failure indicates this duplication violates the quality gate.Extract a common helper that accepts the document spec and a binding-creator callback, or parametrise the shared logic.
Proposed refactor
+def _create_binding_with_document( + client: testing.TestClient, + profile_id: str, + doc_spec: binding_support.DocumentSpec, + create_binding: cabc.Callable[[str], None], +) -> str: + """Create a reference document with one revision and bind it. + + Returns the revision ID. + """ + _, revision_id = binding_support.create_document_with_revision( + client, + profile_id, + doc_spec, + ) + create_binding(revision_id) + return revision_id + + def _create_series_profile_binding( client: testing.TestClient, profile_id: str, episode_id: str, ) -> str: """Create a style-guide document, one revision, and a series-profile binding. Returns the revision ID. """ - _, revision_id = binding_support.create_document_with_revision( - client, - profile_id, - binding_support.DocumentSpec( - kind="style_guide", - name="Resolved series guide", - summary="Resolved series guide", - content_hash="resolved-bindings-series", - ), - ) - binding_support.create_series_binding( - client, - revision_id=revision_id, - profile_id=profile_id, - effective_from_episode_id=episode_id, + return _create_binding_with_document( + client, + profile_id, + binding_support.DocumentSpec( + kind="style_guide", + name="Resolved series guide", + summary="Resolved series guide", + content_hash="resolved-bindings-series", + ), + lambda rev_id: binding_support.create_series_binding( + client, + revision_id=rev_id, + profile_id=profile_id, + effective_from_episode_id=episode_id, + ), ) - return revision_id def _create_episode_template_binding( client: testing.TestClient, profile_id: str, template_id: str, ) -> str: """Create a guest-profile document, one revision, and an episode-template binding. Returns the revision ID. """ - _, revision_id = binding_support.create_document_with_revision( - client, - profile_id, - binding_support.DocumentSpec( - kind="guest_profile", - name="Resolved template guest", - summary="Resolved template guest", - content_hash="resolved-bindings-template", - ), - ) - reference_support.create_reference_binding( - client, - revision_id=revision_id, - template_id=template_id, + return _create_binding_with_document( + client, + profile_id, + binding_support.DocumentSpec( + kind="guest_profile", + name="Resolved template guest", + summary="Resolved template guest", + content_hash="resolved-bindings-template", + ), + lambda rev_id: reference_support.create_reference_binding( + client, + revision_id=rev_id, + template_id=template_id, + ), ) - return revision_idAdd the import for
cabc:if typ.TYPE_CHECKING: import asyncio + import collections.abc as cabc from falcon import testing🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_binding_resolution_api.py` around lines 19-71, extract the duplicated document-and-binding creation pattern from _create_series_profile_binding and _create_episode_template_binding into a new helper _create_binding_with_document that accepts (client, profile_id, doc_spec, create_binding_callback). Update both functions to call this new helper, passing their specific DocumentSpec and a lambda that wraps their binding creation call. Import collections.abc as cabc in the TYPE_CHECKING block to type the callback parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_binding_resolution_api.py` around lines 19 - 71, Duplicate logic in _create_series_profile_binding and _create_episode_template_binding should be extracted into a helper: add _create_binding_with_document(client, profile_id, doc_spec, create_binding_callback) that calls binding_support.create_document_with_revision to get revision_id, then invokes create_binding_callback(client, revision_id) and returns revision_id; update _create_series_profile_binding to call the helper with its specific DocumentSpec and a lambda that calls binding_support.create_series_binding(..., revision_id=revision_id, profile_id=profile_id, effective_from_episode_id=episode_id), and update _create_episode_template_binding to call the helper with its DocumentSpec and a lambda that calls reference_support.create_reference_binding(..., revision_id=revision_id, template_id=template_id); add import collections.abc as cabc inside the TYPE_CHECKING block and annotate the callback parameter using cabc.Callable for typing.episodic/canonical/reference_documents/documents.py (1)
92-109: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand the docstring for
list_reference_documents_pagedto full NumPy style.This public function requires structured documentation. Add Parameters, Returns, and Raises sections consistent with
list_reference_documents(lines 77–89).📝 Suggested docstring structure
async def list_reference_documents_paged( uow: CanonicalUnitOfWork, *, request: ReferenceDocumentListRequest, ) -> tuple[list[ReferenceDocument], int]: - """List reusable reference documents and their unpaginated total.""" + """List reusable reference documents for one owning series profile and their unpaginated total. + + Parameters + ---------- + uow : CanonicalUnitOfWork + Unit of work providing repository access. + request : ReferenceDocumentListRequest + Typed list request containing owner series identifier, optional kind filter, + and pagination values. + + Returns + ------- + tuple[list[ReferenceDocument], int] + Documents matching the requested owner series and pagination window, plus the + unpaginated total count of matching documents. + + Raises + ------ + ReferenceValidationError + If pagination values, owner series identifier, or kind are invalid. + ReferenceEntityNotFoundError + If the owning series does not exist. + """ parsed_owner_id, parsed_kind = await _prepare_document_list_query(uow, request)As per coding guidelines: "Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/reference_documents/documents.py` at line 92, expand the single-line docstring for `list_reference_documents_paged` into a full NumPy-style docstring with Parameters, Returns, and Raises sections; clarify that this variant returns a tuple of (documents list, total count); document `uow` and `request` in Parameters, the tuple structure in Returns, and list ReferenceValidationError and ReferenceEntityNotFoundError in Raises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/reference_documents/documents.py` around lines 92 - 109, Update the docstring for the public function list_reference_documents_paged to a full NumPy-style docstring: replace the single-line summary with a short description and add Parameters (documenting uow: CanonicalUnitOfWork and request: ReferenceDocumentListRequest), a Returns section describing tuple[list[ReferenceDocument], int] (documents for the requested owner series and the unpaginated total), and a Raises section listing ReferenceValidationError and ReferenceEntityNotFoundError; keep wording consistent with the existing list_reference_documents docstring and ensure the docstring sits immediately above the list_reference_documents_paged definition.episodic/canonical/profile_templates/services/_generic.py (2)
273-286: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a full NumPy-style docstring to
list_history_paged.This public function is missing documentation entirely. Add Parameters, Returns, and Raises sections.
📝 Suggested docstring structure
async def list_history_paged( uow: CanonicalUnitOfWork, *, parent_id: uuid.UUID, kind: EntityKind | str, page: Pagination, ) -> tuple[list[object], int]: + """List paged history entries and the unpaginated total for one parent entity. + + Parameters + ---------- + uow : CanonicalUnitOfWork + Unit-of-work providing repositories and transactional boundaries. + parent_id : uuid.UUID + Identifier of the parent entity. + kind : EntityKind | str + Entity kind selector (series profile or episode template). + page : Pagination + Pagination parameters (limit and offset). + + Returns + ------- + tuple[list[object], int] + History entries for the requested parent entity within the pagination window, + plus the unpaginated total count of history entries for that parent. + + Raises + ------ + ValueError + Raised when ``kind`` is unsupported. + """ dispatch = _get_repos_for_kind(uow, kind)As per coding guidelines: "Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/profile_templates/services/_generic.py` at line 273, add a full NumPy-style docstring to `list_history_paged` with Parameters, Returns, and Raises sections; document `uow`, `parent_id`, `kind`, and `page` in Parameters; clarify in Returns that the tuple contains (history items list, total count); list ValueError in Raises for unsupported kind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/profile_templates/services/_generic.py` around lines 273 - 286, Add a NumPy-style docstring to the public function list_history_paged describing its purpose and documenting Parameters (uow: CanonicalUnitOfWork, parent_id: uuid.UUID, kind: EntityKind | str, page: Pagination), Returns (tuple[list[object], int] — the paged history entries and the unpaginated total count), and Raises (ValueError for unsupported kind); place the docstring immediately above the function signature and follow the numpy sections "Parameters", "Returns", and "Raises" wording and formatting as in the project's docstring conventions.
325-340: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a full NumPy-style docstring to
list_entities_with_revisions_paged.This public function is missing documentation entirely. Add Parameters, Returns, and Raises sections mirroring the pattern from
list_entities_with_revisions(lines 289–322).📝 Suggested docstring structure
async def list_entities_with_revisions_paged( uow: CanonicalUnitOfWork, *, kind: EntityKind | str, page: Pagination, series_profile_id: uuid.UUID | None = None, ) -> tuple[list[tuple[object, int]], int]: + """List entities paired with their latest revisions and the unpaginated total. + + Parameters + ---------- + uow : CanonicalUnitOfWork + Unit-of-work providing repositories and transactional boundaries. + kind : EntityKind | str + Entity kind selector (series profile or episode template). + page : Pagination + Pagination parameters (limit and offset). + series_profile_id : uuid.UUID | None, default None + Optional profile filter used for episode-template listing. + + Returns + ------- + tuple[list[tuple[object, int]], int] + Sequence of ``(entity, latest_revision)`` pairs within the pagination window, + plus the unpaginated total count of matching entities. + + Raises + ------ + ValueError + Raised when ``kind`` is unsupported. + """ dispatch = _get_repos_for_kind(uow, kind)As per coding guidelines: "Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/profile_templates/services/_generic.py` at line 325, add a full NumPy-style docstring to `list_entities_with_revisions_paged` with Parameters, Returns, and Raises sections; document `uow`, `kind`, `page`, and `series_profile_id` in Parameters; clarify in Returns that the tuple contains (list of (entity, revision) pairs, total count); list ValueError in Raises for unsupported kind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/profile_templates/services/_generic.py` around lines 325 - 340, Add a full NumPy-style docstring to the public function list_entities_with_revisions_paged describing purpose, Parameters (uow: CanonicalUnitOfWork, kind: EntityKind | str, page: Pagination, series_profile_id: uuid.UUID | None = None), Returns (tuple[list[tuple[object, int]], int] where the list is (entity, latest_revision) pairs and the int is the unpaginated total), and Raises (ValueError for unsupported kind); follow the exact structure and wording style used in list_entities_with_revisions to ensure consistency.episodic/canonical/reference_documents/bindings.py (1)
429-448: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand the docstring for
list_reference_bindings_pagedto full NumPy style.This public function requires structured documentation matching
list_reference_bindings(lines 394–417). Add Parameters, Returns, and Raises sections.📝 Suggested docstring structure
async def list_reference_bindings_paged( uow: CanonicalUnitOfWork, *, request: ReferenceBindingListRequest, ) -> tuple[list[ReferenceBinding], int]: - """List reusable reference bindings and their unpaginated total.""" + """List reusable reference bindings for one target context and their unpaginated total. + + Parameters + ---------- + uow : CanonicalUnitOfWork + Unit of work providing repository access. + request : ReferenceBindingListRequest + Typed list request containing target identifiers and pagination values. + + Returns + ------- + tuple[list[ReferenceBinding], int] + Bindings matching the requested target context and pagination window, + plus the unpaginated total count of matching bindings. + + Raises + ------ + ReferenceValidationError + If pagination values, target kind, or target identifier are invalid. + """ _validate_pagination(request.limit, request.offset)As per coding guidelines: "Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/reference_documents/bindings.py` at line 429, expand the single-line docstring for `list_reference_bindings_paged` into a full NumPy-style docstring with Parameters, Returns, and Raises sections; model it after the existing `list_reference_bindings` docstring (lines 394-417) but clarify that this variant returns a tuple of (bindings list, total count); ensure the Parameters section documents `uow` and `request`, the Returns section documents the tuple structure, and the Raises section lists ReferenceValidationError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/reference_documents/bindings.py` around lines 429 - 448, The docstring for list_reference_bindings_paged is currently a single-line summary; update it to a full NumPy-style docstring (modeled on list_reference_bindings) that includes Parameters (documenting uow: CanonicalUnitOfWork and request: ReferenceBindingListRequest), Returns (tuple[list[ReferenceBinding], int] describing the bindings list and the unpaginated total), and Raises (ReferenceValidationError for invalid pagination/target inputs); keep wording concise and consistent with the existing list_reference_bindings docstring.episodic/canonical/reference_documents/revisions.py (1)
84-99: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand the docstring for
list_reference_document_revisions_pagedto full NumPy style.This public function requires structured documentation. Add Parameters, Returns, and Raises sections mirroring the pattern from
list_reference_document_revisions(lines 70–81).📝 Suggested docstring structure
async def list_reference_document_revisions_paged( uow: CanonicalUnitOfWork, *, request: ReferenceDocumentRevisionListRequest, ) -> tuple[list[ReferenceDocumentRevision], int]: - """List immutable revisions and their unpaginated total.""" + """List immutable revisions for one reference document and their unpaginated total. + + Parameters + ---------- + uow : CanonicalUnitOfWork + Unit of work providing repository access. + request : ReferenceDocumentRevisionListRequest + Typed list request containing document identifiers and pagination values. + + Returns + ------- + tuple[list[ReferenceDocumentRevision], int] + Revisions matching the requested document and pagination window, plus the + unpaginated total count of revisions for that document. + + Raises + ------ + ReferenceValidationError + If pagination values or identifiers are invalid. + ReferenceEntityNotFoundError + If the referenced document does not exist or does not match the owner scope. + """ parsed_document_id = await _prepare_revision_list_query(uow, request)As per coding guidelines: "Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/reference_documents/revisions.py` at line 84, expand the single-line docstring for `list_reference_document_revisions_paged` into a full NumPy-style docstring with Parameters, Returns, and Raises sections; clarify that this variant returns a tuple of (revisions list, total count); document `uow` and `request` in Parameters, the tuple structure in Returns, and list ReferenceValidationError and ReferenceEntityNotFoundError in Raises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@episodic/canonical/reference_documents/revisions.py` around lines 84 - 99, Update the single-line docstring for list_reference_document_revisions_paged to a full NumPy-style docstring matching the pattern used in list_reference_document_revisions: add a one-line summary then Parameters documenting uow: CanonicalUnitOfWork and request: ReferenceDocumentRevisionListRequest, a Returns section describing tuple[list[ReferenceDocumentRevision], int] (revisions for the requested document within the pagination window and the unpaginated total), and a Raises section listing ReferenceValidationError and ReferenceEntityNotFoundError; keep wording consistent with the other function and place the expanded docstring immediately above the function definition in revisions.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/fixtures/llm.py`:
- Line 122: The function _build_adapter currently accepts five parameters but
the noqa suppression removed PLR0913; restore PLR0913 in the noqa list for the
_build_adapter definition (i.e., change "noqa: TD001, TD002" to include PLR0913)
so the linters are consistent with the existing pylint disable, or alternatively
refactor _build_adapter to accept four or fewer arguments and remove both
PLR0913 and the pylint disable if you choose to reduce parameters; target the
async def _build_adapter(...) signature when making the change.
---
Outside diff comments:
In `@episodic/canonical/profile_templates/services/_generic.py`:
- Around line 273-286: Add a NumPy-style docstring to the public function
list_history_paged describing its purpose and documenting Parameters (uow:
CanonicalUnitOfWork, parent_id: uuid.UUID, kind: EntityKind | str, page:
Pagination), Returns (tuple[list[object], int] — the paged history entries and
the unpaginated total count), and Raises (ValueError for unsupported kind);
place the docstring immediately above the function signature and follow the
numpy sections "Parameters", "Returns", and "Raises" wording and formatting as
in the project's docstring conventions.
- Around line 325-340: Add a full NumPy-style docstring to the public function
list_entities_with_revisions_paged describing purpose, Parameters (uow:
CanonicalUnitOfWork, kind: EntityKind | str, page: Pagination,
series_profile_id: uuid.UUID | None = None), Returns (tuple[list[tuple[object,
int]], int] where the list is (entity, latest_revision) pairs and the int is the
unpaginated total), and Raises (ValueError for unsupported kind); follow the
exact structure and wording style used in list_entities_with_revisions to ensure
consistency.
In `@episodic/canonical/reference_documents/bindings.py`:
- Around line 429-448: The docstring for list_reference_bindings_paged is
currently a single-line summary; update it to a full NumPy-style docstring
(modeled on list_reference_bindings) that includes Parameters (documenting uow:
CanonicalUnitOfWork and request: ReferenceBindingListRequest), Returns
(tuple[list[ReferenceBinding], int] describing the bindings list and the
unpaginated total), and Raises (ReferenceValidationError for invalid
pagination/target inputs); keep wording concise and consistent with the existing
list_reference_bindings docstring.
In `@episodic/canonical/reference_documents/documents.py`:
- Around line 92-109: Update the docstring for the public function
list_reference_documents_paged to a full NumPy-style docstring: replace the
single-line summary with a short description and add Parameters (documenting
uow: CanonicalUnitOfWork and request: ReferenceDocumentListRequest), a Returns
section describing tuple[list[ReferenceDocument], int] (documents for the
requested owner series and the unpaginated total), and a Raises section listing
ReferenceValidationError and ReferenceEntityNotFoundError; keep wording
consistent with the existing list_reference_documents docstring and ensure the
docstring sits immediately above the list_reference_documents_paged definition.
In `@episodic/canonical/reference_documents/revisions.py`:
- Around line 84-99: Update the single-line docstring for
list_reference_document_revisions_paged to a full NumPy-style docstring matching
the pattern used in list_reference_document_revisions: add a one-line summary
then Parameters documenting uow: CanonicalUnitOfWork and request:
ReferenceDocumentRevisionListRequest, a Returns section describing
tuple[list[ReferenceDocumentRevision], int] (revisions for the requested
document within the pagination window and the unpaginated total), and a Raises
section listing ReferenceValidationError and ReferenceEntityNotFoundError; keep
wording consistent with the other function and place the expanded docstring
immediately above the function definition in revisions.py.
In `@tests/test_binding_resolution_api.py`:
- Around line 19-71: Duplicate logic in _create_series_profile_binding and
_create_episode_template_binding should be extracted into a helper: add
_create_binding_with_document(client, profile_id, doc_spec,
create_binding_callback) that calls
binding_support.create_document_with_revision to get revision_id, then invokes
create_binding_callback(client, revision_id) and returns revision_id; update
_create_series_profile_binding to call the helper with its specific DocumentSpec
and a lambda that calls binding_support.create_series_binding(...,
revision_id=revision_id, profile_id=profile_id,
effective_from_episode_id=episode_id), and update
_create_episode_template_binding to call the helper with its DocumentSpec and a
lambda that calls reference_support.create_reference_binding(...,
revision_id=revision_id, template_id=template_id); add import collections.abc as
cabc inside the TYPE_CHECKING block and annotate the callback parameter using
cabc.Callable for typing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f88f8737-6ea3-426f-a3d5-c485ac30a76a
📒 Files selected for processing (12)
episodic/api/handlers.pyepisodic/canonical/profile_templates/services/_generic.pyepisodic/canonical/reference_documents/bindings.pyepisodic/canonical/reference_documents/documents.pyepisodic/canonical/reference_documents/revisions.pypyproject.tomltests/fixtures/api.pytests/fixtures/llm.pytests/test_api_error_envelope.pytests/test_binding_resolution_api.pytests/test_profile_template_pagination_api.pytests/test_reference_document_roundtrip.py
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationtests/test_binding_resolution_api.py: What lead to degradation?The module contains 2 functions with similar structure: _create_episode_template_binding,_create_series_profile_binding Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Large Methodepisodic/canonical/profile_templates/services/_generic.py: _get_repos_for_kind What lead to degradation?_get_repos_for_kind has 115 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Python language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat tests/test_binding_resolution_api.pyLength of output: 9213 File: Do not refactor this further. The two helpers were deliberately introduced in the prior extract-function refactor to shorten The binding calls differ in support module, keyword argument set, and domain meaning — Paste the following into the CodeScene suppression dialogue:
|
|
Tip For best results, initiate chat on the files or code changes. File: Do not refactor this. The Large Method finding is a direct and unavoidable consequence of resolving the Code Duplication finding on Paste the following into the CodeScene suppression dialogue:
|
Summary
This branch carries the pre-implementation ExecPlan for roadmap item
4.1.2. The plan covers eight milestones that finalize REST surfaces forthe previously implemented
/v1canonical resources: a unified{code, message, details}error envelope served through a single Falconadd_error_handler, the{items, limit, offset, total}paginationenvelope across every list endpoint, consistent filter parameter
parsing, and an inbound-adapter-local
AuthorizationPortscaffold witha permit-all default. Full Role-Based Access Control (RBAC) and tenancy
isolation remain roadmap item
5.1.No production code changes ship in this branch. The plan authorises the
work that follows after explicit user approval.
Review walkthrough
totalplumbing, error-envelope handler placement, the authorization scaffold location, resolved-bindings pagination, the readiness503exemption, and the deliberate deferral ofIdempotency-Key/Retry-After.total→ pagination retrofit → history pagination → filter consistency → authorization scaffold → docs → gates).Notes
(
{code, message, details}) fromdocs/episodic-tui-api-design.mdrather than RFC 9457 Problem Details verbatim, so the response
Content-Typeremainsapplication/json.survey, hexagonal/architecture guard) contributed the file:line
evidence that underpins the constraints, risks, and milestone
scoping.
References
Summary by Sourcery
Documentation: