feat(acms): implement Temporal Data Model (Revision-Aware RDF) with 3 storage tiers #615
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Blocks
#577 feat(acms): implement Temporal Data Model (Revision-Aware RDF) with 3 storage tiers
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!615
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m6-temporal-data-model-revision-aware-rdf"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements issue #577: Temporal Data Model (Revision-Aware RDF) with 3 storage tiers for the ACMS.
Changes
New Files
src/cleveragents/domain/models/acms/temporal.py— Domain models:TemporalMetadata,TemporalNode,RevisionChain,TierQueryResult,TierRetentionConfig,TemporalBackendprotocolsrc/cleveragents/domain/models/acms/temporal_stubs.py—InMemoryTemporalBackendstubsrc/cleveragents/application/services/temporal_service.py—TemporalServicewith structlog, DIfeatures/temporal_data_model.feature— 61 Behave scenariosfeatures/steps/temporal_data_model_steps.py— Step implementationsrobot/temporal_data_model.robot— 8 Robot Framework integration testsrobot/helper_temporal_data_model.py— Robot helperbenchmarks/temporal_data_model_bench.py— ASV benchmarksdocs/reference/temporal_data_model.md— Reference docsModified Files
src/cleveragents/domain/models/acms/strategy.py—BackendSet.temporaltyped toTemporalBackend | Nonesrc/cleveragents/domain/models/acms/__init__.py— Export all new typessrc/cleveragents/application/services/__init__.py— ExportTemporalServiceCHANGELOG.md— Entry for #577Quality Gates
nox -s lint— Passednox -s typecheck— 0 errorsnox -s security_scan— PassedCloses #577
PM Status Check — PR #615
Author: @hamza.khyari | Milestone: v3.5.0 (M6) | Mergeable: No (conflict) | Reviews: None
Issues
MoSCoW/andPoints/labels.Action Items
The PR body and quality gates look solid. Once the conflict is resolved this should be straightforward to review.
2debeab205d43dfe088bhamza.khyari referenced this pull request2026-03-07 02:29:19 +00:00
d43dfe088b828aa454c3@hamza.khyari This PR has a merge conflict. Please rebase onto master. Branch naming (
feature/) is correct for feature work.PM Status Check — Day 29
Author: @hamza.khyari | Milestone: v3.5.0 (M6) | Mergeable: NO (conflict) | Reviews: None
Current State
Temporal Data Model (Revision-Aware RDF) implementation for issue #577. 61 Behave scenarios, 8 Robot tests, quality gates pass per PR description. However:
Action Required
Priority note: @hamza.khyari — Your PR #610 (repo indexing, M5) has 20 P1 findings with no response. That must be addressed before this PR. M5 is more critical than M6.
828aa454c39322c5ebc9PM Status Check — Day 29
Author: @hamza.khyari | Milestone: v3.6.0 (Post-MVP) | Reviews: None
Current State
Temporal RDF triples implementation. Has a merge conflict — needs rebase onto current master. No reviewer assigned yet.
Action Required
Lower urgency — Post-MVP milestone. @hamza.khyari has 4 open PRs; the M5 PRs (#610, #612) should take priority.
PM Compliance Audit — PR #615
Auditor: PM (automated sweep) | Date: 2026-03-09 | Reference: CONTRIBUTING.md §Pull Request Process (items 1–12)
Checklist
Closes #N/Fixes #N)Closes #577present in bodyType/FeaturepresentReview Status
Merge Readiness
NOT READY — Blockers:
Action items for @hamza.khyari:
Note to reviewers: This is an M6 feature (temporal data model for ACMS). 61 Behave scenarios, 8 Robot tests. Quality gates reported passing by author but not independently verified. Priority: Medium — not on critical path but needed for M6 completion.
PM Escalation — Merge Conflict + No Reviews (Day 29)
@hamza.khyari This PR has had a merge conflict since @freemo flagged it on March 7 and has received zero code reviews in 3 days.
Required actions:
This is an M6 feature (temporal data model). While not on the critical path, it's needed for M6 completion and the lack of reviewer engagement needs to be addressed. Please ping reviewers directly after rebasing.
Code Review — PR #615 (Round 1)
Reviewer: @brent.edwards
Protocol:
code_review_protocol.md— 5-pass lens methodology + adversarial re-readPlaybook:
review_playbook.md— Architecture Review Checklist (domain/service PR)Branch:
feature/m6-temporal-data-model-revision-aware-rdf@9322c5ebIssue: #577 | Author: @hamza.khyari
Scope: 14 files, +3,457 lines (3 source, 1 feature, 1 step, 1 robot, 1 robot helper, 1 benchmark, 1 changelog, 3 init modifications, 1 strategy model modification, 1 docs)
Nox Verification Matrix
linttypecheckunit_testscoverage_reportsecurity_scanwrapping.pysemgrep findings)dead_codeArchitecture Review Checklist
domain/models/acms/application/services/TemporalBackend) defines backend contract — clean DI boundarydomain/models/acms/temporal_stubs.py— proper test doubleBaseModel— immutability enforcedBackendSet.temporaladded asTemporalBackend | None = None— backward-compatible__init__.pyfilesdocs/reference/temporal_data_model.md# type: ignorein any source fileCoverage for PR Files
temporal.py...stubs — expected uncoverabletemporal_stubs.pytemporal_service.py_default_temporal_scope()WARM/COLD branchesFindings
features/steps/temporal_data_model_steps.py[P1]
features/steps/temporal_data_model_steps.py:1-1107— File exceeds 500-line limit (1,107 lines, >2x limit). CONTRIBUTING.md requires files under 500 lines. This step file should be split into at least 2-3 modules (e.g.,temporal_metadata_steps.py,temporal_node_steps.py,temporal_query_steps.py) to comply with the project policy.src/cleveragents/domain/models/acms/temporal_stubs.py[P2]
temporal_stubs.py:153-165,253-262,280-291— Diff coverage at 84.7%, well below 97% threshold. The following code paths inInMemoryTemporalBackendare untested:get_history()RECENT temporal scope branch (lines 153-165)get_revision_chain()cycle guard and missing-predecessor branches (lines 203-205)query_by_tier()WARM tier filtering (lines 254-262)query_by_tier()RECENT temporal scope within tier (lines 281-291)These represent real behavioral paths that should have Behave scenarios exercising them.
[P3]
temporal_stubs.py:34-38—_validate_non_blank()duplicated betweentemporal.py:54-59andtemporal_stubs.py:34-38. The signatures differ slightly (return typestrvsNone, param namefield_namevsparam_name). Consider extracting to a shared private helper in theacmspackage to satisfy DRY.src/cleveragents/application/services/temporal_service.py[P3]
temporal_service.py:350-354—_default_temporal_scope()WARM/COLD branches uncovered. Minor gap — the function mapsContextTier → TemporalScopebut only the HOT branch is exercised. Adding a parameterized scenario or direct unit test would close this.features/steps/temporal_data_model_steps.py[P3]
temporal_data_model_steps.py(5 occurrences) —# type: ignore[misc]suppressions in test steps. These are in scenarios that intentionally assign to frozen Pydantic model fields to verify immutability (e.g.,node.temporal.is_current = False). This is an acceptable test pattern — the assignment must occur for theFrozenInstanceErrorassertion to fire. Noting for completeness.Summary
temporal_stubs.pydiff coverage at 84.7%Overall assessment: This is a well-crafted, architecturally sound PR. The domain models are clean, frozen, and well-documented with spec references. The protocol-based DI boundary is excellent. The service layer follows project conventions with structlog and proper error handling. The single P1 blocker is the step file size — splitting it into 2-3 files should be straightforward. The P2 coverage gap in the stub implementation has untested RECENT/WARM/COLD paths that represent real behavioral branches.
Verdict: REQUEST_CHANGES (1 P1 blocker)
Reviewed per
code_review_protocol.md(5-pass lens) andreview_playbook.md(Architecture Checklist). Adversarial re-read completed — zero additional findings.REQUEST_CHANGES — 1 P1 blocker found.
F1 (P1):
features/steps/temporal_data_model_steps.pyat 1,107 lines exceeds the 500-line CONTRIBUTING.md limit. Split into 2-3 step modules.F2 (P2):
temporal_stubs.pydiff coverage at 84.7% — RECENT scope, cycle guard, and WARM/COLD tier code paths are untested.3 additional P3 nits. See detailed comment #57219 for full findings, nox matrix, and architecture checklist.
Overall: architecturally sound, clean domain models, excellent DI boundaries. Needs the step file split to pass review.
Reviewer Response — @brent.edwards
Responding to PM compliance audit (#57069), PM escalation (#57087), and PM status checks (#56528, #56805).
Review Status
The PM audit (#57069) and escalation (#57087) both noted "No formal code reviews yet" and "zero code reviews in 3 days." This has been resolved:
code_review_protocol.md(5-pass lens) andreview_playbook.md(Architecture Review Checklist).Merge Conflict
The PM flagged a merge conflict. I was able to fetch and review the branch at commit
9322c5ebwithout issues — the branch is currently only 1 commit behind master andmergeable: true. The merge conflict may have been resolved by a prior rebase, or the PM status was stale. @hamza.khyari should confirm merge status before requesting re-review.Findings Summary
0 P0, 1 P1, 1 P2, 3 P3 — One P1 blocker:
[P1]
features/steps/temporal_data_model_steps.pyat 1,107 lines — exceeds the 500-line CONTRIBUTING.md limit by >2x. Needs to be split into 2-3 focused step modules. This is a mechanical refactor — the step definitions are already logically grouped by domain concept.[P2]
temporal_stubs.pydiff coverage at 84.7% — the RECENT scope, cycle guard, and WARM/COLD tier code paths inInMemoryTemporalBackendare untested. These are real behavioral branches that should have Behave scenarios.PM Action Items
CONTRIBUTING Compliance
The PM audit checked all 12 CONTRIBUTING requirements and found all passing. I can confirm from my code review:
Closes #577presentOverall Assessment
This is an architecturally sound PR with clean domain models, excellent DI boundaries, and thorough documentation. The single P1 is a mechanical file split. Once that's done and the P2 coverage gap is addressed, I expect to APPROVE on re-review.
9322c5ebc92c18423c4bReview Response — All 5 Findings Addressed (commit
2c18423c)Zero deferrals. 17 files changed. All checks pass: lint (0 errors), Pyright (0 errors, 0 warnings), 74/74 BDD scenarios (was 67), 97% overall coverage. All source files under 500 lines.
P1 — Step file exceeds 500-line limit — FIXED
temporal_data_model_steps.py(1,107 lines) split into 3 focused modules:temporal_data_model_domain_steps.pytemporal_data_model_backend_steps.pytemporal_data_model_service_steps.pyAll under 500 lines. No step definitions changed. Behave auto-discovers split files.
P2 —
temporal_stubs.pydiff coverage at 84.7% — FIXED (now 98.8%)Added 7 new BDD scenarios covering all previously-untested paths:
get_history()withTemporalScope.RECENT— retention-window cutoff filterget_revision_chain()with cyclic revision chain — cycle guard breakget_revision_chain()with dangling predecessor — missing-predecessor breakquery_by_tier()withContextTier.WARM— warm-tier pool constructionquery_by_tier()withTemporalScope.RECENT— RECENT scope within tier_tier_default_scope(ContextTier.WARM)— RECENT default scope_tier_default_scope(ContextTier.COLD)— ALL default scopeCoverage results:
temporal_stubs.pytemporal_service.py_validation.pyRemaining 9 uncovered lines: 7 protocol
...stubs (uncoverable by design) + 2 list comprehension branches insidequery_by_tierRECENT scope filter.P3 —
_validate_non_blank()duplication — FIXEDExtracted to
src/cleveragents/domain/models/acms/_validation.pywith unified signaturevalidate_non_blank(value, field_name) -> str. Bothtemporal.pyandtemporal_stubs.pynow import from the shared module. The-> strreturn type works for both use cases (Pydantic validator pass-through and fire-and-forget validation).P3 —
_default_temporal_scope()WARM/COLD branches uncovered — FIXEDAdded 2 service scenarios (
WARM tier uses RECENT default scope,COLD tier uses ALL default scope) that exercise both branches.temporal_service.pyis now at 100%.P3 —
# type: ignore[misc]in test steps — Acknowledged5 occurrences intentionally assign to frozen Pydantic model fields to verify
FrozenInstanceError. Acceptable test pattern per reviewer's note.2c18423c4b56cea48ef156cea48ef1dfd0ce6d7bRound 2 — Verification Review (commit
dfd0ce6d)Reviewer: @brent.edwards
Protocol: 5-pass lens review + nox verification matrix
Nox Verification Matrix
linttypechecksecurity_scanwrapping.pyfindingsunit_testscoverage_reportPrior Finding Verification
backend_steps.py(415),domain_steps.py(248),model_steps.py(317),service_steps.py(275). All under 500.temporal_stubs.pyat 84.7% coverage__repr__onTemporalNodeBaseModelauto-generates__repr__.RevisionChain.latestfor empty chaincurrent_uriis required (min_length=1). Empty chains structurally impossible.TierRetentionConfigplaceholder defaultsAll prior findings resolved or verified N/A.
Per-File Coverage
temporal_service.pytemporal_stubs.pytemporal.py_validation.pyFile Compliance
All 7
src/files under 500 lines. Zero# type: ignorein production code. All 4 step files under 500 lines.Fresh 5-Pass Findings (0 P0, 1 P1, 4 P2, 7 P3)
P1:
[P1]
src/cleveragents/domain/models/acms/temporal_stubs.py:79–103—create_revisionsilently corrupts whennew_node.node_uri == current_uri. Line 100 writes the historical node to_nodes[current_uri], then line 103 overwrites it with_nodes[new_node.node_uri](same key). The historical record is lost, and the "new" node hasis_current=Truepointing to itself viais_revision_of. The protocol docstring (temporal.py:368–380) doesn't document this precondition. Fix: Addif new_node.node_uri == current_uri: raise ValueError(...)guard.P2:
[P2]
src/cleveragents/domain/models/acms/temporal_stubs.py:152–154—get_historywithRECENTscope hardcodesTierRetentionConfig()default. Always uses 24h cutoff regardless of caller's custom retention config. In contrast,query_by_tiercorrectly acceptsretentionas parameter. Mismatch between methods.[P2]
src/cleveragents/domain/models/acms/temporal_stubs.py:123,146— Prefix-collision in URI matching.node_uri.startswith(node_uri_base)means a query for"uko-py:class/Auth"matches both"uko-py:class/Auth_v1"and"uko-py:class/AuthManager_v1". Protocol doesn't specify matching semantics.[P2]
src/cleveragents/domain/models/acms/temporal_stubs.py:213–219— O(n·k) forward-walk inget_revision_chain. Re-scans all nodes on each iteration. Acceptable for test stub, but should be documented as known limitation to prevent copying to production backend.[P2]
src/cleveragents/domain/models/acms/temporal.py:54— Scattered import placement._validate_non_blankimported after__all__declaration and section comment, not with other imports at module top. Minor PEP 8 violation.P3:
[P3]
src/cleveragents/application/services/temporal_service.py:36–40— DRY violation:_validate_non_blank()redefined locally with different return-type contract vs_validation.py:8–17.[P3]
src/cleveragents/domain/models/acms/temporal.py:368–380—create_revisionprotocol docstring missing precondition thatnew_node.node_urimust differ fromcurrent_uri.[P3]
src/cleveragents/domain/models/acms/temporal_stubs.py:348–354—store_nodesilently overwrites existing node with same URI. Protocol doesn't specify duplicate behavior.[P3]
src/cleveragents/domain/models/acms/temporal_stubs.py:221—get_revision_chainassumes linear chains. Branching chains produce non-deterministic results.[P3] Missing BDD scenario for
create_revisionwhennew_node.node_uri == current_uri.[P3] Missing BDD scenario for
store_nodeoverwrite behavior.[P3]
CHANGELOG.mdstates "61 Behave scenarios" but feature file contains 74Scenario:entries. Stale count.Summary
Verdict: REQUEST_CHANGES — 1 new P1 blocker:
create_revisionsame-URI corruption bug. The fix is a single guard (if new_node.node_uri == current_uri: raise ValueError(...)) in the stub, plus documenting the precondition in the protocol docstring. All prior Round 1 blockers are resolved. The code quality is excellent overall — zero# type: ignore, strong coverage, clean 4-way step file split.REQUEST_CHANGES — Round 2 verification review on
dfd0ce6d.All prior Round 1 findings resolved: step file split (4 files, all under 500 lines),
temporal_stubs.pycoverage up from 84.7% to 98.8%. Full nox matrix passes (9,802 scenarios, 99% coverage). Excellent work on the split.1 new P1 blocker:
create_revisionintemporal_stubs.py:79–103silently corrupts the revision chain whennew_node.node_uri == current_uri. Historical record is lost, node points to itself. Fix: addif new_node.node_uri == current_uri: raise ValueError(...)guard + document precondition in protocol docstring.4 P2 + 7 P3 advisory findings in comment #57450. Once the same-URI guard is added, this PR is ready to approve.
dfd0ce6d7b82e7fd2fe3Round 2 Response — All 12 Findings Addressed (commit
82e7fd2f)Zero deferrals. All checks pass:
ruff format(0 reformats),ruff check(0 errors), Pyright (0 errors, 0 warnings), 76/76 BDD scenarios, all files under 500 lines.P1 —
create_revisionsame-URI corruption — FIXEDAdded guard in
temporal_stubs.py:82-87:Protocol docstring (
temporal.py:374-381) updated with explicit precondition andValueErrorin Raises section. New BDD scenario: "Create revision with same URI as current raises ValueError".P2 —
get_historyRECENT hardcodes retention — FIXEDAdded optional
retention: TierRetentionConfig | None = Noneparameter toget_history()intemporal_stubs.py:138. WhenNone, falls back to defaultTierRetentionConfig(). Line 161 now useseffective = retention or TierRetentionConfig().P2 — Prefix-collision in URI matching — DOCUMENTED
Added
Note:sections toget_history()andget_current()docstrings documenting thatstr.startswithcauses prefix-collision (e.g.,"Auth"matches"AuthManager") and this is a known stub limitation.P2 — O(n·k) forward-walk in
get_revision_chain— DOCUMENTEDAdded
Note:section documenting the O(n·k) complexity as acceptable for the in-memory test stub, and noting that branching chains produce non-deterministic ordering.P2 — Scattered import placement in
temporal.py— FIXED_validate_non_blankimport moved from after__all__(line 54) to the import block at module top (line 37), alongside othercleveragentsimports.P3 —
temporal_service.pylocal_validate_non_blankDRY — FIXEDRemoved the local definition at lines 36-40 and replaced with
from cleveragents.domain.models.acms._validation import validate_non_blank as _validate_non_blank. All three consumers (temporal.py,temporal_stubs.py,temporal_service.py) now share the single_validation.pyhelper.P3 — Protocol docstring missing precondition — FIXED
See P1 fix above —
create_revisionprotocol docstring now has explicitPrecondition:section.P3 —
store_nodeoverwrite behavior — DOCUMENTEDAdded
Note:to both protocol (temporal.py:430-433) and stub (temporal_stubs.py) docstrings: "Silently overwrites any existing node with the same node_uri." New BDD scenario: "Store node overwrites existing node with same URI".P3 — Linear chain assumption — DOCUMENTED
See O(n·k) fix above —
get_revision_chaindocstring now states: "Assumes linear chains — branching chains produce non-deterministic ordering."P3 — Missing same-URI BDD scenario — FIXED
Added scenario "Create revision with same URI as current raises ValueError" with step
creating a revision from "{uri}" to itself should raise ValueError.P3 — Missing store_node overwrite BDD scenario — FIXED
Added scenario "Store node overwrites existing node with same URI" with steps
I store a replacement nodeandthe stored node resource should be "{expected}".P3 — CHANGELOG stale count — FIXED
Updated from "61 Behave scenarios" to "76 Behave scenarios".
Final Pre-Merge Bug-Hunt — PR #615 (commit
dfd0ce6d)Reviewer: @brent.edwards
Protocol: Exhaustive line-by-line adversarial review of all 5 production source files (1,187 lines total)
Bug Report
BUG 1 —
create_revisionsame-URI → historical record destroyed (CRITICAL)src/cleveragents/domain/models/acms/temporal_stubs.py:100–105Previously reported in Round 2. When
new_node.node_uri == current_uri, line 100 writes the historical node, then line 105 overwrites it with the new node at the same key. Historical record is lost; revision chain points to itself.Fix: Guard at stub:
if new_node.node_uri == current_uri: raise ValueError(...)And at service:
temporal_service.py:106–121— addif current_uri == new_node_uri: raise ValueError(...).BUG 2 —
create_revisionon already-historical node corruptsvalid_until+ creates branch (CRITICAL)src/cleveragents/domain/models/acms/temporal_stubs.py:85–100(NEW)No check that
old_node.temporal.is_current is True. Ifcurrent_urirefers to a node that is already historical (from a prior revision), the existingvalid_untiltimestamp is silently overwritten.Reproduction:
create_revision("A", B, t1)→ A marked historical withvalid_until=t1, B is currentcreate_revision("A", C, t2)→ A'svalid_untilsilently changed fromt1tot2(data corruption). C is stored withis_revision_of=A, creating a branch where both B and C descend from A.This creates a branching revision chain, which
get_revision_chaincannot handle correctly (Bug 5 below).Fix: Add after line 83:
if not old_node.temporal.is_current: raise ValueError("cannot revise a non-current node")BUG 3 —
mark_historicalon already-historical node silently corrupts data (MAJOR)src/cleveragents/application/services/temporal_service.py:303–317(NEW)Neither the service nor the backend checks whether the target node is already historical. Calling
mark_historicalon a node with an existingvalid_untilsilently overwrites the original timestamp.Fix: Service should check
is_currentbefore delegating, or backend should guard it.BUG 4 —
store_nodesilently overwrites existing nodes (MAJOR)src/cleveragents/domain/models/acms/temporal_stubs.py:329–331(NEW)store_nodedoesself._nodes[node.node_uri] = nodewith no duplicate check. A retry loop or duplicate event callingstore_initial_nodetwice with the same URI silently loses the first version.Fix: Add:
if node.node_uri in self._nodes: raise ValueError(f"node {node.node_uri} already exists")BUG 5 —
get_revision_chainassumes positional last = current head (MAJOR)src/cleveragents/domain/models/acms/temporal_stubs.py:211–216(NEW)The forward-walk appends URIs in discovery order, then assumes the last element is the current head. If a branching chain exists (enabled by Bug 2), the result depends on
dictiteration order and is non-deterministic.Also: if
mark_historicalwas called on the head without creating a successor, the chain has no current node, but the last historical element is labeledcurrent_uri.Fix: After building
chain_uris, find the node withis_current=Trueexplicitly.BUG 6 —
get_historyRECENT scope hardcodes 24h retention (MAJOR)src/cleveragents/domain/models/acms/temporal_stubs.py:150–157Previously reported in Round 2.
get_history(..., RECENT)constructs a freshTierRetentionConfig()(24h default) instead of using the service's configured retention. In contrast,query_by_tiercorrectly accepts a retention parameter.Files with Zero Bugs
temporal.py(445 lines) — All models frozen, validators correct, timezone coercion sound_validation.py(17 lines) — Simple and correctstrategy.py(396 lines) —BackendSet.temporaltyping correctSummary
temporal_stubs.pytemporal_stubs.pyvalid_untilcorruption + branchtemporal_service.pymark_historicalon historical node → timestamp corruptiontemporal_stubs.pystore_nodesilently overwrites duplicate URItemporal_stubs.pyget_revision_chainnon-deterministic on branching chainstemporal_stubs.pyget_historyRECENT hardcodes 24h retentionCombined with prior Round 2 findings:
Verdict: This PR has 2 P0 data-corruption bugs and 3 P1 behavioral bugs in
temporal_stubs.py. These are all in the revision lifecycle —create_revision,mark_historical, andstore_nodeall lack precondition guards that prevent corruption of the revision chain.The good news: all fixes are straightforward guard clauses (3–5 lines each). The domain models in
temporal.pyand the validation infrastructure are solid. The bugs are concentrated in the stub implementation and service layer validation gaps.@hamza.khyari — Recommended fix order:
is_currentanduri != current_uriguards tocreate_revision(stub + service)is_currentguard tomark_historical(service + stub)store_nodeis_current=Truelookup instead of positional last inget_revision_chainREQUEST_CHANGES — Final pre-merge bug-hunt on
dfd0ce6d.Exhaustive adversarial review of all 5 production files (1,187 lines). Found 2 P0 data-corruption bugs and 3 P1 behavioral bugs, all in the revision lifecycle:
create_revisionsame-URI → historical record destroyed (temporal_stubs.py:100–105)create_revisionon already-historical node →valid_untilcorruption + branching chain (temporal_stubs.py:85–100)mark_historicalon historical node → silent timestamp corruption (temporal_service.py:303–317)store_nodesilently overwrites duplicate URIs (temporal_stubs.py:329–331)get_revision_chainnon-deterministic on branching chains (temporal_stubs.py:211–216)All fixes are straightforward guard clauses (3–5 lines each). Domain models in
temporal.pyare solid — zero bugs. Full details in comment #57488.Supplemental Bug-Hunt — PR #615 (commit
dfd0ce6d)Reviewer: @brent.edwards
Focus: Net-new bugs not included in any prior review round
New Bugs Found (7)
BUG 7 —
TemporalServiceconstructor acceptsNonebackend (MAJOR)temporal_service.py:73— No guard onbackendparameter.TemporalBackendis a Protocol (structural typing, not enforced at runtime). PassingNonestores it silently; every subsequent method call fails withAttributeError: 'NoneType' has no attribute '...'— opaque and unhelpful.Fix: Add
if backend is None: raise TypeError("backend must not be None")BUG 8 — Timezone-aware non-UTC datetimes silently bypass validators (MAJOR)
temporal.py:98–110— The_ensure_valid_from_tzvalidator only coerces naive datetimes to UTC. A timezone-aware non-UTC datetime (e.g.,UTC-5) passes through unchanged, violating the "UTC timestamp" contract. Any downstream code doing naive comparisons orreplace(tzinfo=None)gets wrong absolute times.Trigger:
TemporalMetadata(valid_from=datetime(2025, 6, 15, 12, 0, tzinfo=timezone(timedelta(hours=-5))))→valid_from.tzinfois EST, not UTC. Stored time is 5 hours off.Fix: Convert non-UTC aware datetimes:
return v.astimezone(UTC)instead ofreturn vBUG 9 — No cross-field validation:
valid_untilbeforevalid_fromaccepted (MAJOR)temporal.py:62–117— No@model_validatorenforcesvalid_from < valid_until. ATemporalMetadatawithvalid_untilearlier thanvalid_fromis silently accepted. This represents an impossible time window ("expired before it started"), corruptingquery_by_tierresults.Trigger:
create_revisionon a node whosevalid_fromis in the future — backend setsvalid_until=now(), producingvalid_from > valid_until.Fix: Add
@model_validator(mode="after")checkingif self.valid_until and self.valid_until < self.valid_from: raise ValueError(...)BUG 10 — No validation that
is_current=Truecontradictsvalid_untilbeing set (MAJOR)temporal.py:62–117—is_current=TrueANDvalid_untilset is semantically contradictory: claims to be current but also superseded. Downstream code uses these fields independently, causing the node to appear in both "current-only" queries (viais_current) AND "historical" window queries (viavalid_until), double-counting it across tiers.Fix: Add to model validator:
if self.is_current and self.valid_until is not None: raise ValueError(...)BUG 11 —
get_revision_chainforward-walk collects nodes from divergent branches (MAJOR)temporal_stubs.py:209–219— The forward-walk collects all nodes whoseis_revision_ofpoints to any node already in the chain, including nodes on sibling branches. This is distinct from previously reported Bug 5 (last=current assumption) — this is a correctness bug where the chain itself contains wrong nodes.Trigger:
A → B(main branch) andA → C(fork).get_revision_chain("uko:B")returns(A, B, C)or(A, C, B)— C should not be in B's chain at all.BUG 12 —
RevisionChainallowscurrent_urito appear inpredecessors(MINOR)temporal.py:222–265— No validation preventscurrent_urifrom duplicating a predecessor.depthovercounts andall_uriscontains duplicates.BUG 13 —
MappingProxyTypewrapping is shallow: nested dicts remain mutable (MINOR)strategy.py:223–231, 283–291—MappingProxyType(v)only freezes the top-level mapping. Nesteddictvalues remain mutable, violating the ADR-004 immutability contract. A consumer can mutateconfig.extra["nested"]["key"] = "new"through the proxy.Updated Totals (All Rounds Combined)
The 4 new P1 findings in
temporal.pyare the most significant. They represent missing model-level invariant enforcement — the domain model silently accepts semantically invalid states. The fixes are all Pydantic validators (5–10 lines each) and would prevent the downstream bugs in the stubs and service layers.82e7fd2fe3d9464e7699Round 3 Response — All 6 Findings Addressed (commit
d9464e76)Zero deferrals. 7 files changed. All checks pass:
ruff format(0 reformats),ruff check(0 errors), Pyright (0 errors, 0 warnings), 81/81 BDD scenarios (was 76), all files under 500 lines.create_revision→ data loss82e7fd2f). Added defense-in-depth guard at service layer (temporal_service.py:117-121).:83-88+ service:117-121create_revisionon historical node →valid_untilcorruptionis_currentguard at stub:91-95mark_historicalon historical node → timestamp corruptionis_currentguard at stub:371-376store_nodesilently overwrites duplicate URI:396-398get_revision_chainpositional last ≠ current headis_current=Truelookup at stub:244-250get_historyRECENT hardcodes 24h retention82e7fd2f). Optionalretentionparam at stub:136,167.New BDD scenarios (5 added, 76 → 81)
Mark historical on already-historical node raises ValueErrorCreate revision on already-historical node raises ValueErrorGet revision chain identifies current head explicitlyTemporalService create revision with same URI raises ValueErrorTemporalService mark historical on already-historical raises ValueErrorProtocol docstrings updated
create_revision: Addedis_currentprecondition alongside existing same-URI preconditionstore_node: Changed from "implementation-defined overwrite" toValueErroron duplicatemark_historical: Added "already historical" to Raises sectionNox verification matrix
ruff format --checkruff checkpyrightbehaveFinal Sweep — Net-New Findings (Round 3)
Branch:
feature/m6-temporal-data-model-revision-aware-rdf@82e7fd2f(force-pushed since Round 2)Full 5-pass lens re-read + adversarial pass across all 17 files on the new HEAD. Note: this branch was rebased and force-pushed since our last review.
Known Bugs FIXED in This Push
The following 8 previously reported bugs are now resolved:
temporal_stubs.pysame-URIcreate_revision→ historical record destroyedValueError(lines 83–88)temporal.pyscattered import placement__all___validate_non_blankredefined_validation.pystore_nodeoverwrite behavior undocumentedcreate_revisionstore_nodeoverwriteRemaining Known Bugs (Still Open)
The following 15 previously reported bugs are still present in the new code:
create_revisionon already-historical node →valid_untilcorruptionmark_historicalon historical node → silent timestamp corruptionstore_nodesilently overwrites duplicate URIsget_revision_chainassumes positional last = current headNonebackend (not enforced at runtime)valid_until < valid_fromis_current=Truecontradictsvalid_untilget_historyRECENT hardcodes 24h retentionget_revision_chainnon-deterministic on branchesRevisionChainallowscurrent_uriinpredecessorsMappingProxyTypewrapping instrategy.pyNet-New Findings
[P1]
temporal_service.py:245+temporal.py:391-395—get_history(RECENT)silently ignores the service's configured retention. TheTemporalBackendprotocol definesget_history(node_uri_base, temporal_scope)with only 2 parameters — noretention. The stub'sget_historyadds a thirdretention: TierRetentionConfig | None = Noneparameter not in the protocol, defaulting toTierRetentionConfig()(24h warm) whenNone.TemporalService.get_historyat line 245 correctly follows the protocol and passes only 2 arguments, so the service'sself._retention(customizable at construction) is never forwarded. Consequence:TemporalService(backend, retention=TierRetentionConfig(warm_retention_hours=48))will haveget_history(base, RECENT)use the 24h default instead of the configured 48h. Contrast withquery_by_tierat line 302 which correctly passesself._retention. Root cause: the protocol'sget_historysignature is missing theretentionparameter. This is distinct from known bug #10 (which described the symptom at the stub level) — this is the protocol-level design bug causing it.[P2]
temporal_stubs.py:59-111—create_revisiondoes not validatenew_node.temporal.is_revision_of == current_uri. The method markscurrent_urias historical and storesnew_nodeas-is, but never checks that the new node'sis_revision_ofactually links back to the superseded node. A direct backend caller (bypassingTemporalService) can passnew_nodewithis_revision_of=Noneor pointing to an unrelated URI, breaking the revision chain invariant: the old node gets marked historical butget_revision_chainwon't connect them. The service layer at line 132 happens to setis_revision_of=current_uricorrectly, but the backend doesn't enforce it.[P2]
temporal_stubs.py:110—create_revisiondoes not validatenew_node.temporal.is_current is True. The new node is stored exactly as provided. If a caller passesnew_nodewithis_current=False, both the old node (now historical) and the new node are non-current, leaving the chain with no current head. Hot-tier queries andget_currentwould returnNonedespite the chain having a valid latest revision. Same layering issue: the service setsis_current=Trueat line 131, but the backend contract doesn't enforce it.[P3]
temporal_stubs.py:34-36— Import after__all__declaration. The_validate_non_blankimport is placed at line 34, after the__all__block at lines 29–31. This is the same scattered-import pattern as known bug #13 (which was fixed intemporal.py) but persists intemporal_stubs.py. PEP 8 and isort expect all imports at module top before__all__.[P3]
temporal.py:91— Deadser_json_timedeltaconfig on a model with notimedeltafields.TemporalMetadatasetsmodel_config = ConfigDict(ser_json_timedelta="iso8601"), but the model only hasdatetime,bool, andstr | Nonefields — notimedelta. This config has zero effect and is misleading about the model's serialization behaviour.Cumulative Summary (Rounds 1–3)
Merge gates: 1 P0 + 8 P1 = 9 blockers remain.
d9464e7699d1cb68529eSupplemental Response — All 7 Findings Addressed (commit
d1cb6852)Zero deferrals. 10 files changed. All checks pass:
ruff format(0 reformats),ruff check(0 errors), Pyright (0 errors, 0 warnings), 88/88 BDD scenarios (was 81), all files under 500 lines.TemporalServiceacceptsNonebackendTypeErrorguard in__init__(temporal_service.py:73)v.astimezone(UTC)in both_ensure_valid_from_tzand_ensure_valid_until_tz(temporal.py:100,112)valid_until < valid_fromaccepted@model_validator(mode="after")inTemporalMetadata(temporal.py:128-134)is_current=Truewithvalid_untilsettemporal.py:135-140)get_revision_chaincollects sibling branch nodestemporal_stubs.py:224-257)RevisionChainallowscurrent_uriinpredecessors@model_validatoronRevisionChain(temporal.py:291-298)MappingProxyTypeshallow freeze instrategy.py_deep_freeze_mapping()helper, applied to both_coerce_extraand_coerce_stats(strategy.py:42-50). Note: pre-existing code, not in our diff.New BDD scenarios (7 added, 81 → 88)
TemporalMetadata converts non-UTC aware datetime to UTCTemporalMetadata rejects valid_until before valid_fromTemporalMetadata rejects is_current with valid_until setRevisionChain rejects current_uri in predecessorsTemporalService rejects None backendGet revision chain excludes sibling branch nodesStrategyConfig extra deep-freezes nested dictsNox verification matrix
ruff format --checkruff checkpyrightbehaveFinal Sweep — Round 4 (on new HEAD
d1cb6852)Branch:
feature/m6-temporal-data-model-revision-aware-rdf@d1cb6852(force-pushed since Round 3)Full 5-pass lens re-read + adversarial pass across all 17 files on the new HEAD.
Known Bugs FIXED in This Push (11 of 20)
Major progress — all P0 and most P1 blockers resolved:
create_revisionon already-historical node → corruptiontemporal_stubs.py:92-97now guardsif not old_node.temporal.is_current: raise ValueErrormark_historicalon historical node → silent corruptiontemporal_stubs.py:378-383— sameis_currentguardstore_nodesilently overwrites duplicate URIstemporal_stubs.py:410-412—if node.node_uri in self._nodes: raise ValueErrorget_revision_chainassumes positional last = headtemporal_stubs.py:258-265— explicitis_currentscan with fallbackNonebackendtemporal_service.py:73-75—if backend is None: raise TypeErrortemporal.py:100-103,112-116—v.astimezone(UTC)for aware datetimesvalid_until < valid_fromacceptedtemporal.py:138-143—model_validatorrejectsis_current=Truecontradictsvalid_untiltemporal.py:144-149—model_validatorrejectstemporal_stubs.py:247-251— takes first successor thenbreakRevisionChainallowscurrent_uriinpredecessorstemporal.py:291-297—_validate_no_duplicate_currentmodel_validatorMappingProxyTypewrapping instrategy.pystrategy.py:43-51—_deep_freeze_mappingrecursively wraps nested dictsRemaining Known Bugs (9 of 20)
get_history(RECENT)ignores configured retention (protocol missingretentionparam)get_historyRECENT hardcodes 24h retention in stubstartswith)create_revisiondoesn't validateis_revision_of == current_uricreate_revisiondoesn't validateis_current is Trueon new nodeget_revision_chainnon-deterministic on branches__all__intemporal_stubs.pyser_json_timedeltaconfig intemporal.pyNet-New Findings (4)
[P1]
temporal_stubs.py:117—create_revisionsilently overwrites existing nodes whose URI matchesnew_node.node_uri. The same-URI guard at line 84 only checksnew_node.node_uri == current_uri. It does NOT check whethernew_node.node_urialready exists under a different key. Line 117 (self._nodes[new_node.node_uri] = new_node) silently overwrites any pre-existing node with that URI. Scenario: store nodes A (current) and X (current, unrelated); thencreate_revision("A", node_with_uri_X, ts)passes all guards and destroys node X. Thestore_nodemethod (line 410) properly guards against duplicates withif node.node_uri in self._nodes: raise ValueError;create_revisionmust add the same check after line 97. The service layer attemporal_service.py:148also lacks this check, so neither layer defends against it.[P2]
temporal_stubs.py:264-265—get_revision_chainproduces semantically invalidRevisionChainwhen all nodes are historical. The fallbackif head_uri is None: head_uri = chain_uris[-1]setscurrent_urito a historical node when no node in the chain hasis_current=True. This is reachable through normal API:store_initial_node("A")thenmark_historical("A")thenget_revision_chain("A")→RevisionChain(current_uri="A")where A hasis_current=False. The resultingcurrent_uriviolates the documented semantic ("URI of the current (active) head node"). Fix: Either raiseValueError/KeyErrorwhen no current head exists, or makeRevisionChain.current_urioptional for fully-historical chains.[P3]
temporal.py:93-104,106-117—mode="before"field validators don't normalize string datetime inputs. The timezone coercion validators gate onisinstance(v, datetime). Withmode="before", if a caller passes a timezone-aware string like"2026-06-15T12:00:00+05:00", the check fails (it's astr, notdatetime), the validator returns it unchanged, and Pydantic's coercion parses it into adatetimeat +05:00 — not UTC. This is a gap in the Bug #6 fix:datetimeobjects are correctly converted to UTC, but ISO-string inputs bypass conversion entirely. Fix: Switch tomode="after"so the validator runs on the already-parseddatetime, or add a string-to-datetime pre-parse step.[P3]
strategy.py:43-51—_deep_freeze_mappingonly freezes nested dicts; mutable lists/sets remain mutable. The recursive freeze checksisinstance(v, (dict, MappingProxyType))but does not handlelist,set, or other mutable containers.StrategyConfig(extra={"plugins": ["a", "b"]})producesMappingProxyType({"plugins": ["a", "b"]})where the inner list is still mutable — callers can.append("c"). This is a residual gap after the Bug #18 fix: nested dicts are now frozen, but nested lists/sets are not. Fix: Convertlist→tupleandset→frozensetduring the recursive walk.Cumulative Summary (Rounds 1–4)
P0 blockers: 0 (all cleared).
P1 blockers: 3 remain (#9 retention protocol gap, #NEW-1 create_revision URI collision, and note that #14/#15 — the
is_revision_of/is_currentvalidation gaps — are P2 because the service layer happens to set them correctly).Excellent progress in this push — the most critical data-corruption bugs are resolved.
Round 5 — Adversarial Re-Read (HEAD
d1cb6852)Five-pass lens protocol complete. Adversarial re-read produced 2 net-new findings (0 P1, 0 P2, 2 P3). Zero new findings expected on next pass — declaring convergence for this HEAD.
Net-New Findings
[P3]
_validation.py:14—str.strip()does not remove zero-width Unicode characters (U+200B ZERO WIDTH SPACE, U+200C ZERO WIDTH NON-JOINER, U+200D ZERO WIDTH JOINER). These are category Cf (format) characters, not Zs (space separator), sostr.isspace()returnsFalsefor them. A string consisting solely of zero-width characters passesvalidate_non_blankand propagates as a visually-empty URI/field value.[P3]
temporal.py:144— Missing symmetric invariant. The_validate_temporal_invariantsmodel validator rejectsis_current=Truewithvalid_untilset, but does not reject the converse:is_current=Falsewithvalid_until=None. This creates a semantically inconsistent state — a node marked historical with no record of when it was superseded — which breaks temporal scope queries (e.g., RECENT tier demotion depends onvalid_untilto compute age). The spec docstring (lines 41940-41957) statesvalid_untilis "Nonefor current (active) nodes", implying the reverse should hold.Cumulative Open Bug Tally
get_historyretention protocol gap, #NEW-1create_revisionoverwrites unrelated nodesstartswith, #12 O(n·k) forward-walk, #14create_revisionnois_revision_ofcheck, #15create_revisionnois_currentcheck__all__, #20 deadser_json_timedelta, #NEW-3mode=beforevalidators miss strings, #NEW-4_deep_freeze_mappingmisses lists, #NEW-14 zero-width Unicode passes validation, #NEW-15 missing symmetricis_current/valid_untilinvariantVerdict: REQUEST_CHANGES stands. Two P1 blockers must be resolved before approval.
d1cb68529ee1e9ba190ce1e9ba190c994454850bRound 4 — Final Sweep Response (commit
99445485)All findings from the Round 4 final sweep have been addressed. Zero deferrals — including all five previously-documented limitations.
New findings fixed:
create_revisionURI collisionif new_node.node_uri in self._nodes: raise ValueErrorguard beforeis_currentcheck.get_revision_chainfully-historical chainValueError("no current node in revision chain")instead of silent fallback.mode="before"mode="after"so Pydantic parses string inputs first, then validator coerces to UTC._deep_freeze_mappingignores lists/sets_deep_freeze_valuehelper:list→tuple,set→frozenset, nesteddict→MappingProxyType.get_historydoesn't pass retentionretentionparam to protocol; service now passesself._retention.__all___validate_non_blankimport above__all__intemporal_stubs.py.ser_json_timedeltaconfigmodel_configline and unusedConfigDictimport.Previously-documented limitations — now fixed:
startswith)_uri_matches_base()— requires_delimiter or exact match after base."Auth"no longer matches"AuthManager_v1"._successorsindex (dict[str, list[str]]) maintained bystore_node/create_revision; forward-walk is now O(k).is_revision_ofvalidation on new_nodeif new_node.temporal.is_revision_of != current_uri: raise ValueError.is_currentvalidation on new_nodeif not new_node.temporal.is_current: raise ValueError.valid_fromfor deterministic behavior.Also fixed: missing step definition for
TierRetentionConfig cold_retention_days=0(pre-existing gap).New BDD scenarios (7): URI collision guard, fully-historical chain, ISO-8601 string coercion, deep-freeze list→tuple, prefix collision prevention, wrong
is_revision_of,is_current=Falserejection.Total: 95 scenarios, 303 steps, all green.
CI:
ruff format✅ |ruff check✅ |pyright0 errors ✅ |behave95/95 ✅ | all files ≤ 500 lines ✅994454850b9f47404fceRound 5 — Adversarial Re-Read Response (commit
9f47404f)Both net-new findings from the Round 5 adversarial re-read have been fixed. Zero deferrals.
validate_non_blankpasses zero-width Unicodeunicodedata.category()check — rejects strings composed of invisible/format characters (U+200B, U+200C, U+200D, U+FEFF, and all Cf/Zs/Cc category chars).is_current/valid_untilinvariantif not self.is_current and self.valid_until is None: raise ValueErrorto_validate_temporal_invariantsmodel validator. A historical node must record when it was superseded. Updated cycle-guard test fixture to includevalid_until.New BDD scenarios (2): zero-width Unicode URI rejection,
is_current=Falsewithoutvalid_untilrejection.Total: 97 scenarios, 305 steps, all green.
CI:
ruff format✅ |ruff check✅ |pyright0 errors ✅ |behave97/97 ✅ | all files < 500 lines ✅Note: The cumulative open bug tally in Round 5 (#57646) listed 15 bugs as open, but those were assessed against
d1cb6852(before our Round 4 response). All 15 were already fixed in99445485(Round 4 response). The only genuinely new items were NEW-14 and NEW-15, both now resolved in this commit.Code Review — PR #615: Temporal Data Model (Revision-Aware RDF)
Reviewer: brent.edwards | Checklist: Architecture + Test + Security
Summary
Solid, well-structured implementation of the temporal data model. The domain models (
temporal.py) are clean with thorough Pydantic validation and cross-field invariant enforcement. The in-memory stub backend (temporal_stubs.py) implements a correct revision chain mechanism with a proper successor index for O(k) forward walks. The service layer (temporal_service.py) adds appropriate logging and input validation. Thestrategy.pydeep-freeze improvement (ADR-004) is a good enhancement.97 Behave scenarios + 8 Robot tests provide strong coverage across models, stub backend, and service layer.
Security check: No credentials, no unsafe deserialization, no external I/O in production code. All inputs validated via Pydantic +
_validate_non_blank. Clean.Findings
temporal.pyRevisionChainmissing uniqueness validation onpredecessorsstrategy.py_deep_freeze_valuedoes not recurse into tuple contentstemporal.py_validate_revision_refuses inline check instead of_validate_non_blanktemporal_stubs.pyget_revision_chainforward-walk tie-breaking comment overstates determinismtemporal_stubs.pyget_revision_chainsilently truncates on missing predecessor — no warning loggedSee inline comments for details.
@ -41,0 +44,4 @@"""Recursively freeze a value: dicts → MappingProxyType, lists → tuples."""if isinstance(v, (dict, MappingProxyType)):source = dict(v) if isinstance(v, MappingProxyType) else vreturn MappingProxyType(P2:should-fix —
_deep_freeze_valuedoes not recurse into tuple contents.Dicts, lists, and sets are recursively frozen, but tuples pass through as-is. If
extracontains a tuple-of-dicts (e.g.,{"key": ({"nested": "dict"},)}), the inner dict remains mutable, violating the ADR-004 deep-freeze intent.Suggested fix — add a tuple branch:
@ -0,0 +112,4 @@return v.replace(tzinfo=UTC)return v.astimezone(UTC)@field_validator("is_revision_of")P3:nit — This uses inline
not v or not v.strip()instead of the shared_validate_non_blank(v, "is_revision_of")helper that theTemporalNodevalidators use. Minor inconsistency; using the shared helper would reduce the chance of divergent validation logic.@ -0,0 +281,4 @@) -> tuple[str, ...]:"""Reject empty or whitespace-only predecessor URIs."""for i, uri in enumerate(v):if not uri or not uri.strip():P2:should-fix —
RevisionChainaccepts duplicate URIs inpredecessors.The
_validate_no_duplicate_currentmodel validator checks thatcurrent_uriis not inpredecessors, but there is no check thatpredecessorsitself contains unique URIs. A chain likeRevisionChain(current_uri="c", predecessors=("a", "a"))would be silently accepted, representing logically invalid data.Suggested fix:
@ -0,0 +288,4 @@# Walk forward from queried_node to current head using the# _successors index (O(k) instead of O(n*k) full scan).tip = node_uriP3:nit — When
pred_uri not in self._nodes(line 291), the backward walk silently stops. This could mask data integrity issues in tests where a predecessor reference points to a node that was never stored. Consider emitting astructlog.warninghere so broken chains are discoverable during test runs.@ -0,0 +312,4 @@# Find the current head explicitly.head_uri: str | None = Nonefor uri in chain_uris:if self._nodes[uri].temporal.is_current:P3:nit — Comment says "deterministic branch choice" but the tie-breaking when two successors have identical
valid_fromtimestamps depends on their insertion order in_successors(the last appended wins since>is strict). This is stable for a given insertion sequence but not truly deterministic in the abstract sense — consider>=with a secondary tiebreaker (e.g., lexicographic URI) or documenting the insertion-order dependency.Re-review Complete — No Additional Findings
Performed a line-by-line re-read of all 3 production files (
temporal.py,temporal_stubs.py,temporal_service.py), thestrategy.pydiff, and all test step files. No additional bugs found beyond the 5 findings in the initial review (review ID 2101).The initial findings stand as-is:
No blockers — this PR is in good shape.
9f47404fce4339543b544339543b545af21daf6fRound 6 Response — Review #2101 (5 findings)
All 5 findings from @brent.edwards re-review have been addressed in commit
5af21daf.RevisionChainmissing uniqueness onpredecessors_validate_chain_integritynow checkslen(predecessors) != len(set(predecessors))in addition to the existingcurrent_uri in predecessorscheck. BDD scenario added: "RevisionChain rejects duplicate predecessors"._deep_freeze_valuedoes not recurse into tuple contentslistandtupleinto a singleisinstance(v, (list, tuple))branch so tuple elements are recursively frozen. BDD scenario added: "Deep-freeze recurses into tuple-of-dicts"._validate_revision_refuses inline check instead of_validate_non_blank_validate_non_blank(v, "is_revision_of"). Also updated_validate_predecessorsfor consistency.max(children)with explicit(valid_from, uri)tuple comparison and updated the inline comment to accurately describe lexicographic tiebreaker behavior.warnings.warn()when a predecessor URI is not found in the store during backward chain walk. BDD scenario added: "Revision chain with missing predecessor stops traversal" (existing scenario now also asserts the warning).CI status: ruff format pass | ruff check pass | Pyright 0 errors | 100 Behave scenarios (309 steps) pass | All files under 499 lines
5af21daf6f2966b8cb35It's been enough rounds. Approve.
2966b8cb35d5f7f15215New commits pushed, approval review dismissed automatically according to repository settings