feat(temporal): refuse later revisions with earlier system time - #122
Conversation
A higher document revision number cannot carry earlier or equal system time (ADR 0002/0013).
|
Warning Review limit reachedNext included review available in 52 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
# Conflicts: # CHANGELOG.md # docs/adr/README.md # docs/research/standards-and-literature.md # docs/validation/temporal-event-foundation.md
|
Current head Validation on this exact head: No merge is requested until a qualifying independent approval and current required checks are present. |
|
Current-head review refresh for b032c85:
|
|
Rebased current head 38542fd onto origin/main. The changelog and any ADR conflict were resolved by retaining both feature and current-main decisions; inherited documentation trailing whitespace was removed. Local merge-tree, git diff --cached --check, and cargo fmt --all -- --check pass. Exact-head hosted checks and required independent approvals remain required before protected merge. |
|
Current-head review completed for exact head
|
…-time-order # Conflicts: # ARCHITECTURE.md # CHANGELOG.md # Cargo.toml # README.md # docs/TRACEABILITY.md # docs/adr/README.md # docs/validation/temporal-event-foundation.md # scripts/check_workspace_contract.py # tests/quality/test_check_docstrings.py
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| pub fn refuse_nonincreasing_system_time( | ||
| earlier: DocumentRevision, | ||
| later: DocumentRevision, | ||
| ) -> Result<(), RevisionOrderError> { | ||
| if revisions_are_increasing(earlier, later)? { | ||
| return Ok(()); | ||
| } | ||
| Err(RevisionOrderError::SystemTimeDidNotIncrease) | ||
| } |
There was a problem hiding this comment.
📝 Info: refuse_nonincreasing_system_time returns InvalidRevisionPayload for equal/backward revision numbers
refuse_nonincreasing_system_time propagates InvalidRevisionPayload (via revisions_are_increasing) when the later revision number is not strictly greater, rather than SystemTimeDidNotIncrease. This is documented ("Returns revision-construction errors, or SystemTimeDidNotIncrease...") and tested, so it is intentional, but callers should be aware that a same-numbered revision pair is rejected as an invalid payload, not as a time-order violation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn refuse_nonincreasing_system_time( | ||
| earlier: DocumentRevision, | ||
| later: DocumentRevision, | ||
| ) -> Result<(), RevisionOrderError> { | ||
| if revisions_are_increasing(earlier, later)? { | ||
| return Ok(()); | ||
| } | ||
| Err(RevisionOrderError::SystemTimeDidNotIncrease) | ||
| } |
There was a problem hiding this comment.
🔍 Error-propagation branch in refuse_nonincreasing_system_time may be untested
In refuse_nonincreasing_system_time (revision.rs), the ? operator on revisions_are_increasing(...) has an implicit error-propagation branch that fires only when the two revision numbers are equal or reversed. Every test call (both the unit test at revision.rs and the integration tests at order_contract.rs) passes strictly increasing revision numbers, so that inner Err path is never exercised through this function. Given the repo's 100% branch-coverage gate (AGENTS.md #8), this is worth confirming against the nightly branch-coverage run; the author claims 12/12 branches, but LLVM branch coverage of the ? desugaring is worth double-checking. Not reported as a bug because it is a coverage/CI concern that will be caught by the pinned coverage gate, not a runtime defect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn revisions_are_increasing( | ||
| earlier: DocumentRevision, | ||
| later: DocumentRevision, | ||
| ) -> Result<bool, RevisionOrderError> { | ||
| if later.revision_number <= earlier.revision_number { | ||
| return Err(RevisionOrderError::InvalidRevisionPayload); | ||
| } | ||
| Ok(later.system_time_seconds > earlier.system_time_seconds) | ||
| } |
There was a problem hiding this comment.
📝 Info: System-time gate is consistent with bitemporal revision semantics
The gate refuses a later revision unless its system_time_seconds strictly increases (revision.rs, 55-58). This is transaction/system-time ordering only and does not constrain event/valid time, so it remains consistent with AGENTS.md #5 (revision edges may point to the past in event time). The strict > comparison also correctly rejects equal system times, matching the PR's stated 'earlier or equal' refusal. No bug; noting because the distinction between system time and event time is the crux of correctness here.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// Return whether `later` has a greater revision number and later system time. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns [`RevisionOrderError::InvalidRevisionPayload`] when `later` is not | ||
| /// a strictly greater revision number than `earlier`. | ||
| pub fn revisions_are_increasing( | ||
| earlier: DocumentRevision, | ||
| later: DocumentRevision, | ||
| ) -> Result<bool, RevisionOrderError> { | ||
| if later.revision_number <= earlier.revision_number { | ||
| return Err(RevisionOrderError::InvalidRevisionPayload); | ||
| } | ||
| Ok(later.system_time_seconds > earlier.system_time_seconds) | ||
| } |
There was a problem hiding this comment.
📝 Info: Doc comment on revisions_are_increasing understates behavior
The doc for revisions_are_increasing at revision.rs says it returns whether later has "a greater revision number and later system time," but the function only returns the system-time comparison (revision.rs); the revision-number condition is enforced by returning an error, not folded into the boolean. This is a documentation/behavior wording mismatch, not a correctness bug, since callers get an Err when revision numbers are non-increasing rather than a false.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
revision_order: a higher revision number cannot carry earlier or equal system time.0008.Claim boundary
#45still owns0007.persistence_postgresinterval CHECKs or recreateencrypted_mapping.Test plan
order_contractfailed withE0432cargo test -p revision_order --offline --lib --testscargo clippy -p revision_order --all-targets --offline -- -D warningscargo llvm-cov -p revision_orderlines 23/23; nightly-2026-08-01 branches 12/12