Skip to content

Commit 23654ba

Browse files
AurelioloAureliolo
committed
fix: address 5 CodeRabbit round-4 items
1 parent f2c4446 commit 23654ba

6 files changed

Lines changed: 41 additions & 14 deletions

File tree

‎src/synthorg/engine/identity/diff.py‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,20 @@ class IdentityFieldChange(BaseModel):
4343

4444
@model_validator(mode="after")
4545
def _validate_change_invariants(self) -> IdentityFieldChange:
46-
if self.change_type == "added" and self.old_value is not None:
47-
msg = "change_type='added' requires old_value=None"
48-
raise ValueError(msg)
49-
if self.change_type == "removed" and self.new_value is not None:
50-
msg = "change_type='removed' requires new_value=None"
51-
raise ValueError(msg)
46+
if self.change_type == "added":
47+
if self.old_value is not None:
48+
msg = "change_type='added' requires old_value=None"
49+
raise ValueError(msg)
50+
if self.new_value is None:
51+
msg = "change_type='added' requires new_value to be set"
52+
raise ValueError(msg)
53+
if self.change_type == "removed":
54+
if self.new_value is not None:
55+
msg = "change_type='removed' requires new_value=None"
56+
raise ValueError(msg)
57+
if self.old_value is None:
58+
msg = "change_type='removed' requires old_value to be set"
59+
raise ValueError(msg)
5260
if self.change_type == "modified" and (
5361
self.old_value is None or self.new_value is None
5462
):

‎src/synthorg/engine/review_gate.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -296,13 +296,13 @@ def _dedupe_criteria(task: Task) -> tuple[str, ...]:
296296
repeated criteria would raise ``ValidationError``.
297297
"""
298298
seen: set[str] = set()
299-
result: tuple[str, ...] = ()
299+
result: list[str] = []
300300
for c in task.acceptance_criteria:
301301
stripped = c.description.strip()
302302
if stripped and stripped not in seen:
303303
seen.add(stripped)
304-
result = (*result, stripped)
305-
return result
304+
result.append(stripped)
305+
return tuple(result)
306306

307307
async def _fetch_charter_metadata(
308308
self,

‎tests/unit/engine/identity/test_diff.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,8 +234,12 @@ class TestIdentityFieldChangeModelValidator:
234234
("modified", '"old"', '"new"', None),
235235
# Invalid: added with old_value present
236236
("added", '"previous"', '"new"', "old_value=None"),
237+
# Invalid: added with new_value missing
238+
("added", None, None, "new_value to be set"),
237239
# Invalid: removed with new_value present
238240
("removed", '"old"', '"should-not-be-here"', "new_value=None"),
241+
# Invalid: removed with old_value missing
242+
("removed", None, None, "old_value to be set"),
239243
# Invalid: modified with None old_value
240244
("modified", None, '"new"', "both old_value and new_value"),
241245
# Invalid: modified with None new_value

‎tests/unit/persistence/sqlite/test_identity_version_repo.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,10 @@ async def test_idempotent_save_with_real_identity(
142142
) -> None:
143143
identity = _make_identity()
144144
v = _make_version(identity=identity)
145-
await repo.save_version(v)
146-
await repo.save_version(v)
145+
first = await repo.save_version(v)
146+
second = await repo.save_version(v)
147+
assert first is True
148+
assert second is False
147149
assert await repo.count_versions("agt-001") == 1
148150

149151
@pytest.mark.unit

‎tests/unit/versioning/test_hashing.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,13 @@
77

88

99
class _Simple(BaseModel):
10-
model_config = ConfigDict(frozen=True)
10+
model_config = ConfigDict(frozen=True, allow_inf_nan=False)
1111
name: str
1212
value: int
1313

1414

1515
class _WithEnum(BaseModel):
16-
model_config = ConfigDict(frozen=True)
16+
model_config = ConfigDict(frozen=True, allow_inf_nan=False)
1717
name: str
1818
tags: tuple[str, ...]
1919

‎tests/unit/versioning/test_models.py‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
"""Tests for the generic VersionSnapshot model."""
22

3-
from datetime import UTC, datetime
3+
from datetime import UTC, datetime, timedelta
44

55
import pytest
66
from pydantic import BaseModel, ConfigDict, ValidationError
@@ -83,6 +83,11 @@ def test_blank_content_hash_rejected(self) -> None:
8383
with pytest.raises(ValidationError):
8484
_make_snapshot(content_hash="")
8585

86+
@pytest.mark.unit
87+
def test_malformed_content_hash_rejected(self) -> None:
88+
with pytest.raises(ValidationError, match="64-character lowercase hex"):
89+
_make_snapshot(content_hash="not-a-sha256-hash")
90+
8691
@pytest.mark.unit
8792
def test_blank_saved_by_rejected(self) -> None:
8893
with pytest.raises(ValidationError):
@@ -93,6 +98,14 @@ def test_naive_datetime_rejected(self) -> None:
9398
with pytest.raises(ValidationError):
9499
_make_snapshot(saved_at=datetime(2026, 4, 7, 12, 0)) # noqa: DTZ001
95100

101+
@pytest.mark.unit
102+
def test_non_utc_aware_datetime_rejected(self) -> None:
103+
from datetime import timezone as tz
104+
105+
non_utc = datetime(2026, 4, 7, 12, 0, tzinfo=tz(timedelta(hours=5)))
106+
with pytest.raises(ValidationError, match="must be UTC"):
107+
_make_snapshot(saved_at=non_utc)
108+
96109
@pytest.mark.unit
97110
def test_version_two_accepted(self) -> None:
98111
s = _make_snapshot(version=2)

0 commit comments

Comments
 (0)