Consolidate metrics/clock ports and tighten checkpoint metrics tests (#115) - #116
Merged
Merged
Conversation
Covers the case where IntegrityError fires and the conflicting checkpoint cannot be recovered through get_by_idempotency_key. Uses unittest.mock.AsyncMock to deterministically simulate the race so the adapter's recovery_failures and latency metrics are exercised without PostgreSQL. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace explicit metrics list comparisons in both test_checkpoint_store_records_checkpoint_metrics and test_checkpoint_store_records_recovery_failure_metrics with syrupy snapshot assertions, following the deterministic-fixture pattern from test_generation_orchestration_snapshots.py. _RecordingMetrics gains an as_snapshot helper that returns a stable dict-of-lists shape; the deterministic _StepClock keeps latency values exact in the snapshot without redaction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove the parallel ChronoClockPort and _PerfCounterChronoClock from episodic.qa.chrono and reuse MonotonicClockPort and PerfCounterClock from episodic.observability instead. This eliminates the duplicate port hierarchy and establishes a single canonical clock port for the codebase. Expose MonotonicClockPort from episodic.qa for external consumers that previously imported ChronoClockPort, and update the developers' guide and module docstrings to document MetricsPort and MonotonicClockPort as the canonical observability ports, with BoundedMetricsPort retained as a narrower structural subtype for feature-specific ports. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
OverviewThis PR centralises observability abstractions, tightens checkpoint-metrics tests with deterministic test doubles and snapshot assertions, and implements a focused unit test for the checkpoint recovery-failure metrics path. It completes the follow-up work for issue Key changesObservability ports consolidation
Test infrastructure and metrics refactor
New and updated tests
Documentation and minor changes
Test plan / checks
Files changed (high level)
Issues closed
WalkthroughConsolidate canonical observability ports ( ChangesObservability port consolidation and test instrumentation
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideConsolidates observability port abstractions around the canonical MetricsPort/MonotonicClockPort, refactors workflow checkpoint tests to shared helpers and syrupy snapshots, and adds targeted metrics coverage for recovery-failure paths. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Move the RecordingMetrics fake, the deterministic StepClock, and the make_checkpoint fixture builder out of tests/canonical_storage/ test_workflow_checkpoints.py and into a new _workflow_checkpoint_support module so the test file stays under the project's 400-line module budget and remains focused on behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The .claude/ directory holds per-session runtime state created by the local Claude Code harness (for example scheduled_tasks.lock). It is not meant to be tracked in source control. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
May 29, 2026 15:38
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e87ab32b8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/canonical_storage/test_workflow_checkpoints.py`:
- Line 62: Rename the test function
test_checkpoint_store_get_returns_none_for_missingmake_checkpoint to remove the
accidental "make" token, e.g. rename it to
test_checkpoint_store_get_returns_none_for_missing_checkpoint so the name
matches the test intent and is searchable; update any references or imports that
call this function (if any) to the new name.
- Around line 173-193: Add an explicit assertion that the recovery lookup path
was exercised by asserting the session lookup was awaited/called when
save_or_reuse triggers the recovery branch: after the await
store.save_or_reuse(checkpoint) (inside the pytest.raises block or immediately
afterwards) assert that session.execute (the mock used for
get_by_idempotency_key lookup) was awaited/called (or that
SqlAlchemyWorkflowCheckpointStore.get_by_idempotency_key was invoked) to ensure
the test explicitly verifies the "get_by_idempotency_key" miss path rather than
inferring it from metrics.
🪄 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: 0e1c3770-46dc-4441-8b69-3a8fa5fd2306
📒 Files selected for processing (9)
.gitignoredocs/developers-guide.mdepisodic/metrics_ports.pyepisodic/observability.pyepisodic/qa/__init__.pyepisodic/qa/chrono.pytests/canonical_storage/__snapshots__/test_workflow_checkpoints.ambrtests/canonical_storage/_workflow_checkpoint_support.pytests/canonical_storage/test_workflow_checkpoints.py
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
|
@coderabbitai Please suggest a fix for the following concern and provide an AI coding agent prompt for the fix: The new |
Owner
|
@coderabbitai Please investigate whether the following issue is valid using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix if applicable: The function name |
This comment was marked as resolved.
This comment was marked as resolved.
Owner
|
@coderabbitai Please investigate whether the following issue is valid using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix if applicable: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n "test_checkpoint_store_get_returns_none" tests/canonical_storage/test_workflow_checkpoints.pyLength of output: 236
async def test_checkpoint_store_get_returns_none_for_missingmake_checkpoint(The Here is the AI coding agent prompt:
The requested changes have been implemented and a pull request has been created: View PR |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== episodic/metrics_ports.py ==="
cat episodic/metrics_ports.py
echo ""
echo "=== episodic/qa/chrono.py ==="
cat episodic/qa/chrono.py
echo ""
echo "=== episodic/observability.py (for MetricsPort) ==="
cat episodic/observability.pyLength of output: 16330
Why the pattern is sound
# episodic/metrics_ports.py
class BoundedMetricsPort(typ.Protocol):
...PEP 544 explicitly mandates that when you want a subclass to remain a protocol (i.e., structurally typed and matchable), you must include class ChronoMetricsPort(BoundedMetricsPort, typ.Protocol): # ✅ Correct — remains a Protocol
...
class ChronoMetricsPort(BoundedMetricsPort): # ❌ Wrong — becomes a concrete class
...Both mypy and pyright handle The codebase already applies this identical pattern in class BoundedValueMetricsPort(BoundedMetricsPort, typ.Protocol):
...That precedent confirms the intent is consistent. No fix is required. |
Move the `IntegrityError` construction and the vanishing-conflict session mock into `make_integrity_error()` and `make_vanishing_conflict_session()` factories in the support module. This removes inline `mock.MagicMock`/`mock.AsyncMock` assembly from the test body and keeps the test module focused on behavioural assertions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Rename `test_checkpoint_store_get_returns_none_for_missingmake_checkpoint` to the intended `test_checkpoint_store_get_returns_none_for_missing_checkpoint`.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== _workflow_checkpoint_support.py ==="
cat tests/canonical_storage/_workflow_checkpoint_support.py
echo ""
echo "=== test_workflow_checkpoints.py (relevant lines) ==="
rg -n "missingmake_checkpoint|missing_checkpoint|make_vanishing|make_integrity_error|recovery_failure_metrics" tests/canonical_storage/test_workflow_checkpoints.py
echo ""
echo "=== Check if helpers exist in support module ==="
rg -n "make_vanishing|make_integrity_error" tests/canonical_storage/_workflow_checkpoint_support.pyLength of output: 5366
No requirements were annulled; none violated en-GB-oxendict conventions or increased cyclomatic complexity. |
Owner
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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 (1)
tests/canonical_storage/test_workflow_checkpoints.py (1)
147-166: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit assertions that the recovery-lookup path was exercised.
The test relies solely on the metrics snapshot to verify the recovery-failure path. Once
make_vanishing_conflict_session()is updated to return both the session and empty-result mocks (see comment on the support file), add assertions verifyingsession.executewas awaited andempty_result.scalar_one_or_nonewas called. This confirms the lookup path ran rather than inferring it from recorded metrics.Proposed fix (after updating the helper)
- store = SqlAlchemyWorkflowCheckpointStore( - typ.cast("AsyncSession", make_vanishing_conflict_session()), + conflict = make_vanishing_conflict_session() + store = SqlAlchemyWorkflowCheckpointStore( + typ.cast("AsyncSession", conflict.session), metrics=metrics, clock=StepClock(), ) with pytest.raises(IntegrityError): await store.save_or_reuse(checkpoint) + conflict.session.execute.assert_awaited_once() + conflict.empty_result.scalar_one_or_none.assert_called_once_with() assert metrics.as_snapshot() == snapshot🤖 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/canonical_storage/test_workflow_checkpoints.py` around lines 147 - 166, Update the test_checkpoint_store_records_recovery_failure_metrics test to explicitly assert the recovery lookup path was executed: after modifying make_vanishing_conflict_session to return (session, empty_result) mocks, call the helper to obtain both, pass the session into SqlAlchemyWorkflowCheckpointStore as before, then add assertions that session.execute.await_count (or session.execute.assert_awaited() depending on mock style) shows it was awaited and that empty_result.scalar_one_or_none was called (e.g., empty_result.scalar_one_or_none.assert_called_once() or await/await_count check). Keep the IntegrityError assertion and the final metrics snapshot assertion; just insert these explicit calls referencing make_vanishing_conflict_session, session.execute, and empty_result.scalar_one_or_none to prove the lookup path ran.
🤖 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/canonical_storage/_workflow_checkpoint_support.py`:
- Around line 108-127: Update make_vanishing_conflict_session to return a small
structured object (e.g., a `@dataclass` or simple namespace) that exposes both the
MagicMock session and the empty_result mock so tests can assert calls on them;
specifically keep the existing behavior of session.begin_nested, session.flush
(side_effect=make_integrity_error()), session.execute (returning empty_result),
but return something like {session: session, empty_result: empty_result} (or a
dataclass with attributes session and empty_result) and update any
tests/call-sites to reference the returned_object.session and
returned_object.empty_result when asserting call counts or call args.
---
Outside diff comments:
In `@tests/canonical_storage/test_workflow_checkpoints.py`:
- Around line 147-166: Update the
test_checkpoint_store_records_recovery_failure_metrics test to explicitly assert
the recovery lookup path was executed: after modifying
make_vanishing_conflict_session to return (session, empty_result) mocks, call
the helper to obtain both, pass the session into
SqlAlchemyWorkflowCheckpointStore as before, then add assertions that
session.execute.await_count (or session.execute.assert_awaited() depending on
mock style) shows it was awaited and that empty_result.scalar_one_or_none was
called (e.g., empty_result.scalar_one_or_none.assert_called_once() or
await/await_count check). Keep the IntegrityError assertion and the final
metrics snapshot assertion; just insert these explicit calls referencing
make_vanishing_conflict_session, session.execute, and
empty_result.scalar_one_or_none to prove the lookup path ran.
🪄 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: feb28a15-8197-48cd-8557-2ea262b5255e
📒 Files selected for processing (3)
docs/execplans/upgrade-python-to-3-14-adopt-concurrent-interpreters.mdtests/canonical_storage/_workflow_checkpoint_support.pytests/canonical_storage/test_workflow_checkpoints.py
…ssion Return a `types.SimpleNamespace` instead of the raw session mock so callers can assert that the recovery lookup path was exercised (``session.execute`` awaited and ``empty_result.scalar_one_or_none`` called).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…overy test Replace `types.SimpleNamespace` with a frozen `VanishingConflictSession` dataclass so the return type is explicit. Switch `await_count` to `assert_awaited_once()` and `assert_called_once()` to `assert_called_once_with()` for clearer failure messages.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (2 errors, 5 warnings)
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== _workflow_checkpoint_support.py ==="
cat tests/canonical_storage/_workflow_checkpoint_support.py
echo ""
echo "=== test_workflow_checkpoints.py (recovery_failure test) ==="
rg -n -A 40 "def test_checkpoint_store_records_recovery_failure" tests/canonical_storage/test_workflow_checkpoints.py
echo ""
echo "=== Check for VanishingConflictSession ==="
rg -n "VanishingConflictSession" tests/canonical_storage/_workflow_checkpoint_support.py tests/canonical_storage/test_workflow_checkpoints.pyLength of output: 7418
No further work is required for this PR. |
Owner
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
Contributor
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 `@tests/canonical_storage/_workflow_checkpoint_support.py`:
- Line 109: Update the dataclass decorator to enable slots by changing
dataclasses.dataclass(frozen=True) to dataclasses.dataclass(frozen=True,
slots=True) for the dataclass defined at the decorator on the shown line (i.e.,
the class decorated with dataclasses.dataclass); ensure the signature is updated
everywhere that decorator is used for this class.
🪄 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: d77417db-14d7-4cd0-9a64-939720204d84
📒 Files selected for processing (2)
tests/canonical_storage/_workflow_checkpoint_support.pytests/canonical_storage/test_workflow_checkpoints.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/canonical_storage/_workflow_checkpoint_support.py (1)
1-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the module docstring to reference actual class and function names.
The docstring references
`_RecordingMetrics`,`_StepClock`, and`_checkpoint`but the actual identifiers areRecordingMetrics(line 43),StepClock(line 88), andmake_checkpoint(line 21). Correct the backticked references to match the code.📝 Proposed fix
"""Shared test doubles for workflow checkpoint store tests. -Hosts the deterministic ``_RecordingMetrics`` and ``_StepClock`` fakes used by -``test_workflow_checkpoints.py`` and the ``_checkpoint`` fixture builder, so +Hosts the deterministic ``RecordingMetrics`` and ``StepClock`` fakes used by +``test_workflow_checkpoints.py`` and the ``make_checkpoint`` fixture builder, so the test module stays focused on behaviour rather than infrastructure. """🤖 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/canonical_storage/_workflow_checkpoint_support.py` at lines 3-4, update the module docstring to replace backticked references `_RecordingMetrics`, `_StepClock`, and `_checkpoint` with the actual identifiers used in the module: `RecordingMetrics`, `StepClock`, and `make_checkpoint` respectively; keep all other docstring text unchanged.🤖 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/canonical_storage/_workflow_checkpoint_support.py` around lines 1 - 6, Update the module docstring so the backticked identifiers match the actual symbols in the file: replace `_RecordingMetrics` with RecordingMetrics, `_StepClock` with StepClock, and `_checkpoint` with make_checkpoint (remove the extra leading underscores and use the exact names RecordingMetrics, StepClock, make_checkpoint in backticks). Ensure only the docstring text is changed and the rest of the file is untouched.
🤖 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.
Outside diff comments:
In `@tests/canonical_storage/_workflow_checkpoint_support.py`:
- Around line 1-6: Update the module docstring so the backticked identifiers
match the actual symbols in the file: replace `_RecordingMetrics` with
RecordingMetrics, `_StepClock` with StepClock, and `_checkpoint` with
make_checkpoint (remove the extra leading underscores and use the exact names
RecordingMetrics, StepClock, make_checkpoint in backticks). Ensure only the
docstring text is changed and the rest of the file is untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e34c3c99-76df-4b29-988d-082c885dabfc
📒 Files selected for processing (1)
tests/canonical_storage/_workflow_checkpoint_support.py
lodyai Bot
pushed a commit
that referenced
this pull request
May 30, 2026
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.
leynos
added a commit
that referenced
this pull request
May 31, 2026
* Plan REST surface hardening for 4.1.2
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.
* Ignore `.claude/` Claude Code harness state
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/`.
* Refresh ExecPlan references for Hecate migration
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.
* Add REST error envelope handling
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 reference list totals
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 pagination to profile lists
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.
* Add pagination to history endpoints
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.
* Normalize REST filter parsing
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.
* Add REST authorization scaffold
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.
* Align REST surface documentation
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.
* Align lifespan test with pagination envelope
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.
* Mark REST surface roadmap item complete
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.
* Normalize ExecPlan Oxford spellings
Apply en-GB Oxford `-ize` spellings consistently in the 4.1.2
ExecPlan after review flagged mixed suffix forms.
* Normalize remaining ExecPlan spellings to Oxford -ize
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.
* Dedupe `.claude/` gitignore entry after rebase
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.
* Remove stray blank line in users guide
`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.
* Address reviewer findings on REST surface hardening
Apply the still-valid inline review comments:
- `docs/developers-guide.md`: switch `serialised` to `serialized` to
match the en-GB Oxford `-ize` convention used elsewhere in the file.
- `episodic/api/authorization.py`: replace the `typ.assert_never` arm of
the `AuthorizationDecision` match with an explicit logged
`service_unavailable` 503 envelope so an unexpected enum value cannot
escape as an unstructured 500.
- `episodic/api/errors.py`: add a `# parsed as tuple in Python 3`
comment beside the `except IndexError, ValueError:` clause to flag
the construct's modern semantics (the project's `ruff format`
normalises this back to the bare form, so parenthesisation is not an
option here).
- `episodic/canonical/profile_templates/services/_generic.py`: add a
short justification to the `# noqa: PLR0913` on
`list_history_paged`, matching the sibling
`list_entities_with_revisions_paged` directly below.
- `episodic/canonical/storage/repositories.py`: append the record `id`
as a deterministic tie-breaker to `created_at` in both
`SqlAlchemySeriesProfileRepository.list` and
`SqlAlchemyEpisodeTemplateRepository.list` so paginated lists do not
drift when timestamps tie.
- `tests/api_fixtures.py`: introduce `_assert_total_at_least` and use
it in both list-assertion helpers so the pagination envelope's
`total` is verified as `>= len(items)` rather than `== len(items)`,
matching the contract where `total` is the full filtered count and
`items` is a page slice.
- `tests/test_api_authorization.py`: collapse the three
decision-specific tests into one `pytest.mark.parametrize` over
`(adapter_factory, expected_status, expected_payload)`. The
health-bypass test stays separate because it asserts a different
property.
- `tests/test_binding_resolution_api.py` and
`tests/test_reference_document_roundtrip.py`: replace bare asserts
on pagination envelopes with message-bearing forms that include the
actual/expected values and the offending payload for debuggability.
Skip the four outside-diff refactor suggestions
(`handle_update_entity` split, `parse_pagination` helper extraction,
`_get_repos_for_kind` per-kind factories, `test_api_error_envelope.py`
parametrisation). They expand scope without fixing a bug, and the
user's "keep changes minimal" directive overrides; each remains a
reasonable follow-up if the project later wants the structural change.
* Revert unrelated mdformat reflow in docs
Restore four paragraphs in `docs/developers-guide.md`, two in
`docs/users-guide.md`, and one in
`docs/episodic-podcast-generation-system-design.md` to their
`origin/main` form. None of these paragraphs carry intentional `4.1.2`
content; they were touched only by a wider `make fmt` run that rewrapped
line breaks before/after parenthesised continuations or added stray
leading spaces. The four substantive doc additions for `4.1.2` (the
REST error contract, pagination/filter contract, and authorization
scaffold sections in the developer guide; the `/v1` REST API reference
section in the user guide; the `/v1` route prefix update and pagination
envelope update in the system-design reference REST API specification;
and the roadmap status flip) remain in place.
After this revert, `docs/users-guide.md` is purely additive against
`origin/main`, and the doc-diff stats shrink accordingly.
* Resolve CodeScene findings on REST surface refactor
Address five CodeScene findings raised on the 4.1.2 work without
introducing new lint suppressions. Each finding is fixed by reshaping
the code, not by silencing the rule.
- `handle_http_error` (5 args, max 4): replace
`app.add_error_handler(falcon.HTTPError, handle_http_error)` with
`app.set_error_serializer(serialize_http_error)`. The serializer
hook has a 3-arg `(req, resp, exc)` signature, the function is
synchronous (Falcon ASGI accepts both sync and async serializers,
and the body performs no I/O), and the existing
`noqa: PLR0913, PLR0917, RUF029` suppressions and pylint disables go
away.
- `handle_update_entity` (70 lines, threshold 70): extract
`_require_payload_fields(payload, required_fields)` and
`_raise_mapped_update_error(exc, *, entity_id, payload)` so the
public function is orchestration-only (~15 lines). The new helpers
preserve the optimistic-lock context that
`map_profile_template_error` needs.
- `_pagination_type_error_field` (Bumpy Road, two nested
`try/except`): remove the helper. Replace its callers with a single
`_parse_int_query_param(raw_value, *, name, default)` that returns
the default when the parameter is absent and raises
`validation_error` with the per-field name on parse failure. The
resulting `parse_pagination` reads top-to-bottom with no nesting.
- `_assert_error_envelope` (5 args): introduce a frozen `_Expected`
dataclass and pass it positionally. The helper becomes
`(response, expected)` and all seven call sites supply the same
`_Expected(...)` value.
- `list_entities_with_revisions_paged` and `list_history_paged`
(5 args each): introduce a shared `Pagination` value object at
`episodic/canonical/pagination.py` (added to the `domain_ports`
Hecate group) carrying `(limit, offset)`. Both services now accept
`page: Pagination`, and `parse_pagination` returns a `Pagination`
instance so API resources pass it through unchanged. `noqa: PLR0913`
on both services is removed.
A downstream effect of the `Pagination` value object: `handle_get_history`
went from 7 positional args to 6, then to 2 by bundling the per-call
inputs into a new `HistoryRequest` dataclass (entity id, id field name,
service, serializer, page). Its `noqa: PLR0913, PLR0917` suppression is
removed.
Tests updated:
- `tests/test_api_error_envelope.py`: all seven calls now pass
`_Expected(...)` rather than keyword arguments.
- `tests/test_reference_document_validation.py`: pagination-type
parametrize cases now expect the field-specific messages
("limit must be an integer.", "offset must be an integer.") that
the new `_parse_int_query_param` produces.
Gates: `make check-fmt`, `make lint` (incl. `hecate check`, ruff,
pylint 10.00/10), `make typecheck`, `make markdownlint`, `make test`
(782 passed, 3 skipped).
* Address second reviewer pass on REST surface refactor
Inline findings:
- `episodic/api/handlers.py`: `_raise_mapped_update_error` no longer
accepts a raw `payload`. It now takes the parsed
`expected_revision: int | None` from `update_request.expected_revision`,
so the `revision_conflict` envelope's `details.expected_revision` is
always a typed integer and not a raw payload value.
- `episodic/canonical/reference_documents/bindings.py`,
`documents.py`, `revisions.py`: the non-paged service functions
(`list_reference_bindings`, `list_reference_documents`,
`list_reference_document_revisions`) no longer call the `*_paged`
variant and discard the total. Each now performs its own list-only
repository call, sharing validation/lookup with the paged variant
through a small private helper (`_prepare_*_list_query`). This
removes one unnecessary `count_for_*` round-trip per call from any
caller that does not need the total.
- `tests/test_api_error_envelope.py`:
- the pagination test now sends a valid all-zeros UUID for
`target_id` so the assertion focuses purely on the `limit` range
check rather than relying on the (correct but coincidental)
validation order;
- `_assert_error_envelope` now asserts `payload["details"]` is a
dict before casting, with an explicit failure message that names
the actual type.
- `tests/test_profile_template_pagination_api.py`: replaced the
cross-module private-helper imports of `_create_profile` /
`_create_template` with a `canonical_api_creators` pytest fixture
(defined in `tests/fixtures/api.py`) that bundles both creators in
a frozen `CanonicalApiCreators` dataclass. Bundling them keeps the
test signature within Pylint's default `max-args=4`.
Duplicate finding:
- `episodic/canonical/profile_templates/services/_generic.py`:
extracted the per-kind branches of `_get_repos_for_kind` into
`_build_series_profile_dispatch(uow)` and
`_build_episode_template_dispatch(uow)`. The dispatcher is now a
thin five-line match; each branch helper owns its casts, nested
async wrappers, and dispatch-table construction.
Skipped outside-diff finding (with reason):
- `episodic/api/resources/resolved_bindings.py` push-pagination into
`resolve_bindings`. The current pre-load-then-slice approach is
correct for the expected dataset size (a handful of reference
bindings per series). Pushing pagination into `resolve_bindings`
would require changing the domain service signature, adding new
repository methods, and answering non-trivial questions about how
to count and page the result of a multi-stage in-memory grouping
algorithm. That belongs to a dedicated design pass once dataset
growth makes it material, not to this commit.
Config:
- `pyproject.toml`: extend `[tool.ruff.lint.per-file-ignores]` so
`tests/fixtures/*.py` shares the lenient ruleset already applied to
`**/test_*.py` and `tests/steps/*.py`. These files are pytest
plugins, functionally test code; the existing config simply did not
cover them by glob.
- `tests/fixtures/llm.py`: drop now-redundant `noqa: PLR0913` from
`_build_adapter` (covered by the new fixtures ignore).
Gates: `make check-fmt`, `make lint` (incl. `hecate check`, ruff,
pylint 10.00/10), `make typecheck`, `make markdownlint`, `make test`
(782 passed, 3 skipped).
* Extract page-total assertions from round-trip test
`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.
* Extract helpers from resolved-bindings round-trip test
`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.
* Parametrise canonical error-envelope tests
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.
* Inline kind dispatch builders into `_get_repos_for_kind`
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.
This was referenced Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
test_checkpoint_store_records_recovery_failure_metrics, a mock-driven test that covers therecovery_failuremetrics path when anIntegrityErrorfires and the conflicting checkpoint cannot be recovered viaget_by_idempotency_key.tests/canonical_storage/test_workflow_checkpoints.pyto syrupy snapshots, using_StepClockto keep latency values deterministic and reusable from the helper_RecordingMetrics.as_snapshot(). This follows the pattern already established intests/test_generation_orchestration_snapshots.py.ChronoClockPortand_PerfCounterChronoClockfromepisodic/qa/chrono.pyand reusing the canonicalMonotonicClockPortandPerfCounterClockfromepisodic/observability.py. Re-exportsMonotonicClockPortfromepisodic.qafor any consumers that previously importedChronoClockPortthrough that surface.MetricsPortandMonotonicClockPortports — and the narrowerBoundedMetricsPortstructural subtype — inepisodic/observability.py,episodic/metrics_ports.py, theChronoMetricsPortdocstring, anddocs/developers-guide.md.Closes #115.
Test plan
pytest tests/canonical_storage/test_workflow_checkpoints.py(12 passed, 2 snapshots passed).pytest tests/test_chrono.py tests/test_chrono_contracts.py tests/test_chrono_properties.py tests/test_chrono_langgraph.py tests/steps/test_chrono_steps.py tests/test_port_contracts.py(79 passed).uv run ruff check/uv run ruff format --checkon changed files.uv tool run ty==0.0.32 check(whole repo).uv run hecate check(architecture boundaries).markdownlint-cli2 docs/developers-guide.md.🤖 Generated with Claude Code
Summary by Sourcery
Consolidate observability port abstractions around canonical metrics and monotonic clock interfaces, and tighten workflow checkpoint metrics coverage and fixtures.
New Features:
Enhancements:
Tests: