📝 CodeRabbit Chat: Implement requested code changes - #119
📝 CodeRabbit Chat: Implement requested code changes#119coderabbitai[bot] wants to merge 6 commits into
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>
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>
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>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRenames a test case for clarity and removes an unused variable in the workflow checkpoints tests to match requested review feedback. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
factoryvariable is still used inasync with SqlAlchemyUnitOfWork(factory) as uow:but its assignment was removed, which will cause aNameError; either restore the casted assignment or update the context manager to usesession_factorydirectly with appropriate typing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `factory` variable is still used in `async with SqlAlchemyUnitOfWork(factory) as uow:` but its assignment was removed, which will cause a `NameError`; either restore the casted assignment or update the context manager to use `session_factory` directly with appropriate typing.
## Individual Comments
### Comment 1
<location path="tests/canonical_storage/test_workflow_checkpoints.py" line_range="62-68" />
<code_context>
@pytest.mark.asyncio
-async def test_checkpoint_store_get_returns_none_for_missingmake_checkpoint(
+async def test_checkpoint_store_get_returns_none_for_missing_checkpoint(
session_factory: object,
) -> None:
"""`get` should return None when the checkpoint does not exist."""
- factory = typ.cast("async_sessionmaker[AsyncSession]", session_factory)
+
async with SqlAlchemyUnitOfWork(factory) as uow:
result = await uow.workflow_checkpoints.get(str(uuid.uuid4()))
</code_context>
<issue_to_address>
**issue (testing):** This test no longer defines `factory`, so it will fail before exercising the behaviour it’s meant to validate.
With `factory = typ.cast(...)` removed but `factory` still used in the `SqlAlchemyUnitOfWork` context manager, this test will raise a `NameError` and never exercise the `get` call or its assertion. Please reintroduce the `factory` initialization (or pass `session_factory` directly) so the test actually checks that `get` returns `None` for a missing checkpoint.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| async def test_checkpoint_store_get_returns_none_for_missing_checkpoint( | ||
| session_factory: object, | ||
| ) -> None: | ||
| """`get` should return None when the checkpoint does not exist.""" | ||
| factory = typ.cast("async_sessionmaker[AsyncSession]", session_factory) | ||
|
|
||
|
|
||
| async with SqlAlchemyUnitOfWork(factory) as uow: |
There was a problem hiding this comment.
issue (testing): This test no longer defines factory, so it will fail before exercising the behaviour it’s meant to validate.
With factory = typ.cast(...) removed but factory still used in the SqlAlchemyUnitOfWork context manager, this test will raise a NameError and never exercise the get call or its assertion. Please reintroduce the factory initialization (or pass session_factory directly) so the test actually checks that get returns None for a missing checkpoint.
Code changes was requested by @leynos.
The following files were modified:
tests/canonical_storage/test_workflow_checkpoints.pySummary by Sourcery
Tests: