Skip to content

Commit bd8bcfe

Browse files
AurelioloAureliolo
committed
fix: address CodeRabbit round-6 review findings on PR #1072
Addresses 3 valid findings CodeRabbit posted on the round-5 commit. 1. src/synthorg/persistence/sqlite/decision_repo.py: reject naive datetimes explicitly in ``append_with_next_version``. The parameter type is ``AwareDatetime`` but there is no runtime enforcement at the function boundary -- a naive datetime would silently pass through ``astimezone(UTC)`` (which assumes local time for naive inputs) and produce a wall-clock-disagreeing timestamp in the persisted row. Fail fast with ``ValueError`` and a clear message before any SQL runs. 2. tests/unit/api/fakes_decisions.py: mirror the same naive-datetime rejection in ``FakeDecisionRepository`` so tests observe identical error behavior from the fake and the real backend. Also deep-copy the caller's metadata before wrapping in ``MappingProxyType``, matching production behavior where ``_build_insert_params``'s ``json.dumps`` step produces an independent snapshot decoupled from the caller's dict. Without the deep copy, a test that mutates its source metadata dict after calling the fake would see the mutation reflected in the stored record -- divergent from the real repo which stores a serialized snapshot. 3. tests/unit/api/fakes.py -> tests/unit/api/fakes_backend.py: extract ``FakePersistenceBackend`` (145 lines) into a new sibling module so ``fakes.py`` drops from 843 -> 705 lines, back under the 800-line CLAUDE.md budget. Re-exported from ``fakes`` at the BOTTOM of the module (after all other Fake* classes are defined) so ``from tests.unit.api.fakes import FakePersistenceBackend`` call sites -- of which there are many across the test suite -- keep working unchanged. The sibling-then-reexport pattern avoids a circular import: ``fakes_backend`` imports the repo classes from ``fakes``, and ``fakes`` only imports ``FakePersistenceBackend`` from ``fakes_backend`` at module end, after its own classes are defined. Test plan: - 14,267 unit tests pass - ruff check + format clean - mypy strict clean (1551 source files) - fakes.py: 705 lines (down from 843, under the 800 budget)
1 parent aa521bc commit bd8bcfe

4 files changed

Lines changed: 231 additions & 155 deletions

File tree

‎src/synthorg/persistence/sqlite/decision_repo.py‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -201,13 +201,24 @@ async def append_with_next_version( # noqa: PLR0913
201201
metadata_view: MappingProxyType[str, object] = MappingProxyType(
202202
dict(metadata or {})
203203
)
204+
# Reject naive datetimes explicitly. The parameter type is
205+
# ``AwareDatetime``, which Pydantic validates at model
206+
# boundaries -- but this function accepts it as a raw
207+
# argument, so there's no runtime enforcement until the
208+
# draft ``DecisionRecord`` is constructed. A naive datetime
209+
# passed through ``astimezone(UTC)`` would silently convert
210+
# assuming local time, producing a timestamp that disagrees
211+
# with the caller's actual wall clock. Fail fast instead.
212+
if recorded_at.tzinfo is None:
213+
msg = (
214+
f"recorded_at must be timezone-aware, got a naive "
215+
f"datetime for decision record {record_id!r}"
216+
)
217+
raise ValueError(msg)
204218
# Normalize recorded_at to UTC up-front so the draft record,
205219
# the INSERT parameters, and any subsequent read-back through
206220
# ``get``/``list_by_task``/``list_by_agent`` all carry the same
207-
# timestamp. Without this, a non-UTC input would cause
208-
# ``append_with_next_version`` to return a record whose
209-
# ``recorded_at`` disagrees with what ``get(record_id)`` later
210-
# reads back from the UTC-normalized row.
221+
# timestamp.
211222
recorded_at_utc = recorded_at.astimezone(UTC)
212223
try:
213224
draft_record = DecisionRecord(

‎tests/unit/api/fakes.py‎

Lines changed: 12 additions & 150 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,6 @@
3333
from synthorg.persistence.preset_repository import PresetListRow, PresetRow
3434
from synthorg.security.models import AuditEntry, AuditVerdictStr
3535
from synthorg.security.timeout.parked_context import ParkedContext
36-
from tests.unit.api.fakes_workflow import (
37-
FakeWorkflowDefinitionRepository,
38-
FakeWorkflowExecutionRepository,
39-
FakeWorkflowVersionRepository,
40-
)
4136

4237

4338
class FakeTaskRepository:
@@ -581,151 +576,6 @@ async def count(self) -> int:
581576
return len(self._presets)
582577

583578

584-
class FakePersistenceBackend:
585-
"""In-memory persistence backend for tests."""
586-
587-
def __init__(self) -> None:
588-
self._artifacts = FakeArtifactRepository()
589-
self._projects = FakeProjectRepository()
590-
self._custom_presets = FakePersonalityPresetRepository()
591-
self._workflow_definitions = FakeWorkflowDefinitionRepository()
592-
self._workflow_executions = FakeWorkflowExecutionRepository()
593-
self._workflow_versions = FakeWorkflowVersionRepository()
594-
self._tasks = FakeTaskRepository()
595-
self._cost_records = FakeCostRecordRepository()
596-
self._messages = FakeMessageRepository()
597-
self._lifecycle_events = FakeLifecycleEventRepository()
598-
self._task_metrics = FakeTaskMetricRepository()
599-
self._collaboration_metrics = FakeCollaborationMetricRepository()
600-
self._parked_contexts = FakeParkedContextRepository()
601-
self._audit_entries = FakeAuditRepository()
602-
self._decision_records = FakeDecisionRepository()
603-
self._users = FakeUserRepository()
604-
self._api_keys = FakeApiKeyRepository()
605-
self._checkpoints = FakeCheckpointRepository()
606-
self._heartbeats = FakeHeartbeatRepository()
607-
self._agent_states = FakeAgentStateRepository()
608-
self._settings_repo = FakeSettingsRepository()
609-
# Legacy flat KV store for get_setting/set_setting (pre-namespaced).
610-
# The `settings` property returns `_settings_repo` (namespaced repo).
611-
self._settings: dict[str, str] = {}
612-
self._connected = False
613-
614-
async def connect(self) -> None:
615-
self._connected = True
616-
617-
async def disconnect(self) -> None:
618-
self._connected = False
619-
620-
def get_db(self) -> Any:
621-
msg = "FakePersistenceBackend does not expose a real DB"
622-
raise NotImplementedError(msg)
623-
624-
async def health_check(self) -> bool:
625-
return self._connected
626-
627-
async def migrate(self) -> None:
628-
pass
629-
630-
@property
631-
def is_connected(self) -> bool:
632-
return self._connected
633-
634-
@property
635-
def backend_name(self) -> str:
636-
return "fake"
637-
638-
@property
639-
def artifacts(self) -> FakeArtifactRepository:
640-
return self._artifacts
641-
642-
@property
643-
def projects(self) -> FakeProjectRepository:
644-
return self._projects
645-
646-
@property
647-
def tasks(self) -> FakeTaskRepository:
648-
return self._tasks
649-
650-
@property
651-
def cost_records(self) -> FakeCostRecordRepository:
652-
return self._cost_records
653-
654-
@property
655-
def messages(self) -> FakeMessageRepository:
656-
return self._messages
657-
658-
@property
659-
def lifecycle_events(self) -> FakeLifecycleEventRepository:
660-
return self._lifecycle_events
661-
662-
@property
663-
def task_metrics(self) -> FakeTaskMetricRepository:
664-
return self._task_metrics
665-
666-
@property
667-
def collaboration_metrics(self) -> FakeCollaborationMetricRepository:
668-
return self._collaboration_metrics
669-
670-
@property
671-
def parked_contexts(self) -> FakeParkedContextRepository:
672-
return self._parked_contexts
673-
674-
@property
675-
def audit_entries(self) -> FakeAuditRepository:
676-
return self._audit_entries
677-
678-
@property
679-
def decision_records(self) -> FakeDecisionRepository:
680-
return self._decision_records
681-
682-
@property
683-
def users(self) -> FakeUserRepository:
684-
return self._users
685-
686-
@property
687-
def api_keys(self) -> FakeApiKeyRepository:
688-
return self._api_keys
689-
690-
@property
691-
def checkpoints(self) -> FakeCheckpointRepository:
692-
return self._checkpoints
693-
694-
@property
695-
def heartbeats(self) -> FakeHeartbeatRepository:
696-
return self._heartbeats
697-
698-
@property
699-
def agent_states(self) -> FakeAgentStateRepository:
700-
return self._agent_states
701-
702-
@property
703-
def settings(self) -> FakeSettingsRepository:
704-
return self._settings_repo
705-
706-
@property
707-
def custom_presets(self) -> FakePersonalityPresetRepository:
708-
return self._custom_presets
709-
710-
@property
711-
def workflow_definitions(self) -> FakeWorkflowDefinitionRepository:
712-
return self._workflow_definitions
713-
714-
@property
715-
def workflow_executions(self) -> FakeWorkflowExecutionRepository:
716-
return self._workflow_executions
717-
718-
@property
719-
def workflow_versions(self) -> FakeWorkflowVersionRepository:
720-
return self._workflow_versions
721-
722-
async def get_setting(self, key: str) -> str | None:
723-
return self._settings.get(key)
724-
725-
async def set_setting(self, key: str, value: str) -> None:
726-
self._settings[key] = value
727-
728-
729579
class FakeSettingsRepository:
730580
"""In-memory namespaced settings repository for tests."""
731581

@@ -841,3 +691,15 @@ async def get_channel_history(
841691
limit: int | None = None,
842692
) -> tuple[Message, ...]:
843693
return ()
694+
695+
696+
# FakePersistenceBackend lives in a sibling module to keep this file
697+
# under the 800-line limit. Imported at the BOTTOM of this module so
698+
# the Fake*Repository classes it depends on are already defined by
699+
# the time ``fakes_backend`` is loaded. Re-exported under its
700+
# original name so existing test imports
701+
# (``from tests.unit.api.fakes import FakePersistenceBackend``) keep
702+
# working.
703+
from tests.unit.api.fakes_backend import ( # noqa: E402
704+
FakePersistenceBackend as FakePersistenceBackend, # noqa: PLC0414
705+
)

‎tests/unit/api/fakes_backend.py‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
"""In-memory ``PersistenceBackend`` fake for tests.
2+
3+
Extracted from ``tests/unit/api/fakes.py`` to keep that module under
4+
the 800-line budget. Re-exported from ``fakes`` for backwards
5+
compatibility so existing test imports keep working.
6+
"""
7+
8+
from typing import Any
9+
10+
from tests.unit.api.fakes import (
11+
FakeAgentStateRepository,
12+
FakeApiKeyRepository,
13+
FakeArtifactRepository,
14+
FakeAuditRepository,
15+
FakeCheckpointRepository,
16+
FakeCollaborationMetricRepository,
17+
FakeCostRecordRepository,
18+
FakeDecisionRepository,
19+
FakeHeartbeatRepository,
20+
FakeLifecycleEventRepository,
21+
FakeMessageRepository,
22+
FakeParkedContextRepository,
23+
FakePersonalityPresetRepository,
24+
FakeProjectRepository,
25+
FakeSettingsRepository,
26+
FakeTaskMetricRepository,
27+
FakeTaskRepository,
28+
FakeUserRepository,
29+
)
30+
from tests.unit.api.fakes_workflow import (
31+
FakeWorkflowDefinitionRepository,
32+
FakeWorkflowExecutionRepository,
33+
FakeWorkflowVersionRepository,
34+
)
35+
36+
__all__ = ["FakePersistenceBackend"]
37+
38+
39+
class FakePersistenceBackend:
40+
"""In-memory persistence backend for tests."""
41+
42+
def __init__(self) -> None:
43+
self._artifacts = FakeArtifactRepository()
44+
self._projects = FakeProjectRepository()
45+
self._custom_presets = FakePersonalityPresetRepository()
46+
self._workflow_definitions = FakeWorkflowDefinitionRepository()
47+
self._workflow_executions = FakeWorkflowExecutionRepository()
48+
self._workflow_versions = FakeWorkflowVersionRepository()
49+
self._tasks = FakeTaskRepository()
50+
self._cost_records = FakeCostRecordRepository()
51+
self._messages = FakeMessageRepository()
52+
self._lifecycle_events = FakeLifecycleEventRepository()
53+
self._task_metrics = FakeTaskMetricRepository()
54+
self._collaboration_metrics = FakeCollaborationMetricRepository()
55+
self._parked_contexts = FakeParkedContextRepository()
56+
self._audit_entries = FakeAuditRepository()
57+
self._decision_records = FakeDecisionRepository()
58+
self._users = FakeUserRepository()
59+
self._api_keys = FakeApiKeyRepository()
60+
self._checkpoints = FakeCheckpointRepository()
61+
self._heartbeats = FakeHeartbeatRepository()
62+
self._agent_states = FakeAgentStateRepository()
63+
self._settings_repo = FakeSettingsRepository()
64+
# Legacy flat KV store for get_setting/set_setting (pre-namespaced).
65+
# The `settings` property returns `_settings_repo` (namespaced repo).
66+
self._settings: dict[str, str] = {}
67+
self._connected = False
68+
69+
async def connect(self) -> None:
70+
self._connected = True
71+
72+
async def disconnect(self) -> None:
73+
self._connected = False
74+
75+
def get_db(self) -> Any:
76+
msg = "FakePersistenceBackend does not expose a real DB"
77+
raise NotImplementedError(msg)
78+
79+
async def health_check(self) -> bool:
80+
return self._connected
81+
82+
async def migrate(self) -> None:
83+
pass
84+
85+
@property
86+
def is_connected(self) -> bool:
87+
return self._connected
88+
89+
@property
90+
def backend_name(self) -> str:
91+
return "fake"
92+
93+
@property
94+
def artifacts(self) -> FakeArtifactRepository:
95+
return self._artifacts
96+
97+
@property
98+
def projects(self) -> FakeProjectRepository:
99+
return self._projects
100+
101+
@property
102+
def tasks(self) -> FakeTaskRepository:
103+
return self._tasks
104+
105+
@property
106+
def cost_records(self) -> FakeCostRecordRepository:
107+
return self._cost_records
108+
109+
@property
110+
def messages(self) -> FakeMessageRepository:
111+
return self._messages
112+
113+
@property
114+
def lifecycle_events(self) -> FakeLifecycleEventRepository:
115+
return self._lifecycle_events
116+
117+
@property
118+
def task_metrics(self) -> FakeTaskMetricRepository:
119+
return self._task_metrics
120+
121+
@property
122+
def collaboration_metrics(self) -> FakeCollaborationMetricRepository:
123+
return self._collaboration_metrics
124+
125+
@property
126+
def parked_contexts(self) -> FakeParkedContextRepository:
127+
return self._parked_contexts
128+
129+
@property
130+
def audit_entries(self) -> FakeAuditRepository:
131+
return self._audit_entries
132+
133+
@property
134+
def decision_records(self) -> FakeDecisionRepository:
135+
return self._decision_records
136+
137+
@property
138+
def users(self) -> FakeUserRepository:
139+
return self._users
140+
141+
@property
142+
def api_keys(self) -> FakeApiKeyRepository:
143+
return self._api_keys
144+
145+
@property
146+
def checkpoints(self) -> FakeCheckpointRepository:
147+
return self._checkpoints
148+
149+
@property
150+
def heartbeats(self) -> FakeHeartbeatRepository:
151+
return self._heartbeats
152+
153+
@property
154+
def agent_states(self) -> FakeAgentStateRepository:
155+
return self._agent_states
156+
157+
@property
158+
def settings(self) -> FakeSettingsRepository:
159+
return self._settings_repo
160+
161+
@property
162+
def custom_presets(self) -> FakePersonalityPresetRepository:
163+
return self._custom_presets
164+
165+
@property
166+
def workflow_definitions(self) -> FakeWorkflowDefinitionRepository:
167+
return self._workflow_definitions
168+
169+
@property
170+
def workflow_executions(self) -> FakeWorkflowExecutionRepository:
171+
return self._workflow_executions
172+
173+
@property
174+
def workflow_versions(self) -> FakeWorkflowVersionRepository:
175+
return self._workflow_versions
176+
177+
async def get_setting(self, key: str) -> str | None:
178+
return self._settings.get(key)
179+
180+
async def set_setting(self, key: str, value: str) -> None:
181+
self._settings[key] = value

0 commit comments

Comments
 (0)