Skip to content

Commit 3afe16b

Browse files
week_3: address CodeRabbit review on #937
Resolve the CodeRabbit review comments on the Module C PR: - schemas.py: enforce RFC format parity — AnyUrl for url fields, datetime for committed_at/filtered_at/classified_at/created_at - section_validator.py: replace assert with a typed MalformedKnowledgeItemError guard (survives python -O) - knowledge_source.py: skip+log malformed JSONL rows instead of aborting the whole iteration on ValidationError - config_loader_test.py: isolate os.environ and assert the specific FrozenInstanceError - dataset_test.py: add a timeout to the determinism subprocess call - build_golden_dataset.py: rename unused loop var node_id -> _node_id - section_validator_test.py: assert committed_at as the typed datetime
1 parent 2453895 commit 3afe16b

7 files changed

Lines changed: 37 additions & 15 deletions

File tree

application/tests/librarian/config_loader_test.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import os
22
import unittest
3+
from dataclasses import FrozenInstanceError
34
from unittest import mock
45

56
from application.utils.librarian.config_loader import LibrarianConfig, load_config
@@ -18,8 +19,9 @@ def test_defaults_when_env_unset(self):
1819
self.assertEqual(cfg.conformal_alpha, 0.10)
1920

2021
def test_config_is_frozen(self):
21-
cfg = load_config()
22-
with self.assertRaises(Exception):
22+
with mock.patch.dict(os.environ, {}, clear=True):
23+
cfg = load_config()
24+
with self.assertRaises(FrozenInstanceError):
2325
cfg.link_threshold = 0.5 # type: ignore[misc]
2426

2527

application/tests/librarian/dataset_test.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ def test_build_check_matches_committed_dataset(self):
111111
[sys.executable, _BUILD_SCRIPT, "--check"],
112112
capture_output=True,
113113
text=True,
114+
timeout=120,
114115
)
115116
self.assertEqual(
116117
result.returncode,

application/tests/librarian/section_validator_test.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
"""
77

88
import unittest
9+
from datetime import datetime, timezone
910

1011
from pydantic import ValidationError
1112

@@ -86,7 +87,10 @@ def test_valid_row_builds_section_with_synthesized_identity(self) -> None:
8687
section.artifact_id, "art:OWASP/ASVS:4.0/en/0x11-V2-Authentication.md"
8788
)
8889
self.assertEqual(section.source.repo, "OWASP/ASVS")
89-
self.assertEqual(section.source.committed_at, "2026-05-25T02:25:00Z")
90+
self.assertEqual(
91+
section.source.committed_at,
92+
datetime(2026, 5, 25, 2, 25, tzinfo=timezone.utc),
93+
)
9094
self.assertEqual(section.locator.path, "4.0/en/0x11-V2-Authentication.md")
9195
self.assertEqual(section.language, "en")
9296

application/utils/librarian/knowledge_source.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,16 @@
77
"""
88

99
import json
10+
import logging
1011
from abc import ABC, abstractmethod
1112
from typing import Iterator
1213

14+
from pydantic import ValidationError
15+
1316
from application.utils.librarian.schemas import KnowledgeQueueItem
1417

18+
logger = logging.getLogger(__name__)
19+
1520

1621
class KnowledgeSource(ABC):
1722
@abstractmethod
@@ -28,7 +33,15 @@ def __init__(self, jsonl_path: str) -> None:
2833

2934
def items(self) -> Iterator[KnowledgeQueueItem]:
3035
with open(self._path, encoding="utf-8") as fh:
31-
for line in fh:
36+
for line_no, line in enumerate(fh, start=1):
3237
line = line.strip()
3338
if line:
34-
yield KnowledgeQueueItem.model_validate_json(line)
39+
try:
40+
yield KnowledgeQueueItem.model_validate_json(line)
41+
except ValidationError as exc:
42+
logger.warning(
43+
"Skipping malformed knowledge_queue row at line %d: %s",
44+
line_no,
45+
exc,
46+
)
47+
continue

application/utils/librarian/schemas.py

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,11 @@
1212
from __future__ import annotations
1313

1414
import re
15+
from datetime import datetime
1516
from enum import Enum
1617
from typing import List, Literal, Optional
1718

18-
from pydantic import BaseModel, ConfigDict, Field, model_validator
19+
from pydantic import AnyUrl, BaseModel, ConfigDict, Field, model_validator
1920

2021
SCHEMA_VERSION = "0.2.0"
2122
_SCHEMA_VERSION_RE = re.compile(r"^0\.\d+\.\d+$")
@@ -64,10 +65,10 @@ class SourceRef(BaseModel):
6465
model_config = ConfigDict(extra="forbid")
6566
type: SourceType
6667
repo: Optional[str] = None
67-
url: Optional[str] = None
68+
url: Optional[AnyUrl] = None
6869
commit_sha: Optional[str] = Field(default=None, min_length=7)
6970
commit_message: Optional[str] = None
70-
committed_at: str
71+
committed_at: datetime
7172
author_login: Optional[str] = None
7273

7374
@model_validator(mode="after")
@@ -84,7 +85,7 @@ class Locator(BaseModel):
8485
kind: LocatorKind
8586
id: str = Field(min_length=1)
8687
path: Optional[str] = None
87-
url: Optional[str] = None
88+
url: Optional[AnyUrl] = None
8889
title: Optional[str] = None
8990

9091
@model_validator(mode="after")
@@ -201,7 +202,7 @@ class KnowledgeItem(BaseModel):
201202
artifact_id: str
202203
event_id: str
203204
pipeline_run_id: str
204-
filtered_at: str
205+
filtered_at: datetime
205206
status: KnowledgeStatus
206207
source: SourceRef
207208
locator: Locator
@@ -228,7 +229,7 @@ class LinkProposal(BaseModel):
228229
chunk_id: str
229230
artifact_id: str
230231
pipeline_run_id: str
231-
classified_at: str
232+
classified_at: datetime
232233
status: Literal["linked"] = "linked"
233234
knowledge: KnowledgeSnapshot
234235
retrieval: RetrievalAudit
@@ -251,7 +252,7 @@ class ReviewItem(BaseModel):
251252
chunk_id: str
252253
artifact_id: str
253254
pipeline_run_id: str
254-
created_at: str
255+
created_at: datetime
255256
status: Literal["review_required"] = "review_required"
256257
reason_code: ReasonCode
257258
knowledge: KnowledgeSnapshot

application/utils/librarian/section_validator.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,9 @@ def section_from_knowledge_item(
160160
raise NotKnowledgeError(
161161
f"status={item.status.value!r}; only 'accepted' items may be linked"
162162
)
163-
# status=accepted guarantees content (enforced by the KnowledgeItem model).
164-
assert item.content is not None
163+
# Keep boundary behavior typed even for pre-built/mutated model instances.
164+
if item.content is None:
165+
raise MalformedKnowledgeItemError("status='accepted' requires content")
165166
_require_text(item.content.text)
166167
language = _require_language(item.content.language)
167168

scripts/build_golden_dataset.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ def build_positive_multilink(conn: sqlite3.Connection) -> List[Dict]:
258258
"""
259259
).fetchall()
260260
out = []
261-
for node_id, name, section_id, text, cre_concat in rows:
261+
for _node_id, name, section_id, text, cre_concat in rows:
262262
cre_ids = sorted(set(cre_concat.split("|")))
263263
std = "OTHER"
264264
if "Top 10" in name:

0 commit comments

Comments
 (0)