Decide Sync heads disposition vector coverage#75
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive test-vector support for Phase-1 sync heads disposition. It adds deterministic test cases covering start-sequence derivation, invalid head-sequence validation, overflow handling, response truncation logic, and head-map consistency classification, along with corresponding Python validation code, documentation, and manifest registration. ChangesSync Heads Disposition Test Vectors
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
Agent Runner Summary
Checks
Scope Gate
TDD Red Phase
Builder Self-Review
Reviewer Tests
Auto-Fix
Review Matrix
Review Findings
Review Finding Coverage
Spec Feedback
GitHub Review Findings
GitHub Review Thread Resolution
Integration Status
Program Merge Context
Audit Artifacts
Human Gates
No auto-merge was performed. |
antontranelis
added
spec:change
Contributor
Author
|
@coderabbitai full review Follow-up fix pushed after local runner review: removed the non-UUID deviceId carve-out wording from the Sync heads disposition notes. Local |
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/validate_test_vectors.py`:
- Around line 306-358: derive_sync_start_seq and compare_sync_heads must reject
non-canonical deviceId keys instead of accepting them; add a fast-fail check
that validates the device id format for the incoming device_id and for every key
in the heads maps before proceeding (use or add a helper like
is_canonical_device_id or a regex check), and raise a ValueError (e.g.
"invalid-device-id") when any key is not canonical; update derive_sync_start_seq
to validate the single device_id and all keys of heads, and update
compare_sync_heads to validate all keys in left and right prior to the existing
seq checks/comparisons.
In `@test-vectors/phase-1-interop.json`:
- Around line 579-694: Add explicit test vectors that cover non-canonical /
non-UUID head keys: insert cases into "derive_start_seq_cases" (e.g., heads with
a non-UUID string key and an empty-string key) that assert the same start-seq
behavior or an "invalid-head-key" error as appropriate; add cases to
"invalid_head_seq_cases" (or create a new "invalid_head_key_cases" array) with
heads containing numeric keys, empty string keys, and malformed UUID-like keys
to assert the correct rejection error (e.g., "invalid-head-key"); and add
corresponding entries to "heads_comparison_cases" where one side contains an
invalid key to assert the expected_disposition (e.g., "divergent" or an error
disposition). Ensure each new case uses the same naming pattern (e.g.,
"invalid-device-key-empty-string", "invalid-device-key-non-uuid",
"invalid-device-key-numeric") so they are discoverable and cover the key-shape
rule explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 97c03c70-2cc1-49cf-b411-fb893dc514b5
📒 Files selected for processing (4)
conformance/manifest.jsonscripts/validate_test_vectors.pytest-vectors/README.mdtest-vectors/phase-1-interop.json
Comment on lines
+306
to
+358
| def derive_sync_start_seq(heads: dict, device_id: str): | ||
| if device_id not in heads: | ||
| return 0 | ||
| head = heads[device_id] | ||
| if not is_sync_head_seq(head): | ||
| raise ValueError("invalid-head-seq") | ||
| if head == MAX_SAFE_INTEGER: | ||
| raise ValueError("sync-head-seq-overflow") | ||
| return head + 1 | ||
|
|
||
|
|
||
| def evaluate_sync_response_disposition(response: dict) -> str: | ||
| return "request-next-page" if response["truncated"] else "complete" | ||
|
|
||
|
|
||
| def compare_sync_heads(left: dict, right: dict) -> str: | ||
| for seq in list(left.values()) + list(right.values()): | ||
| if not is_sync_head_seq(seq): | ||
| raise ValueError("invalid-head-seq") | ||
| if set(left) != set(right): | ||
| return "divergent" | ||
| for device_id, seq in left.items(): | ||
| if right[device_id] != seq: | ||
| return "divergent" | ||
| return "consistent" | ||
|
|
||
|
|
||
| def verify_sync_heads_disposition(vector: dict) -> None: | ||
| for case in vector["derive_start_seq_cases"]: | ||
| assert derive_sync_start_seq(case["heads"], case["deviceId"]) == case["expected_start_seq"], case["name"] | ||
|
|
||
| for case in vector["invalid_head_seq_cases"]: | ||
| try: | ||
| derive_sync_start_seq(case["heads"], case["deviceId"]) | ||
| except ValueError as exc: | ||
| assert str(exc) == case["expected_error"], case["name"] | ||
| continue | ||
| raise AssertionError(f"invalid head seq accepted: {case['name']}") | ||
|
|
||
| for case in vector["derive_start_seq_overflow_cases"]: | ||
| try: | ||
| derive_sync_start_seq(case["heads"], case["deviceId"]) | ||
| except ValueError as exc: | ||
| assert str(exc) == case["expected_error"], case["name"] | ||
| continue | ||
| raise AssertionError(f"overflow case accepted: {case['name']}") | ||
|
|
||
| for case in vector["response_truncation_cases"]: | ||
| assert evaluate_sync_response_disposition(case["response"]) == case["expected_disposition"], case["name"] | ||
|
|
||
| for case in vector["heads_comparison_cases"]: | ||
| assert compare_sync_heads(case["left"], case["right"]) == case["expected_disposition"], case["name"] | ||
|
|
There was a problem hiding this comment.
Enforce canonical deviceId keys during sync heads validation.
derive_sync_start_seq and compare_sync_heads currently accept arbitrary head-map keys. Invalid key formats should fail fast instead of being treated as normal data.
🔧 Suggested fix
MAX_SAFE_INTEGER = (1 << 53) - 1
+UUID_V4_LOWER_RE = re.compile(
+ r"^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$"
+)
@@
def is_sync_head_seq(value) -> bool:
@@
return type(value) is int and 0 <= value <= MAX_SAFE_INTEGER
+def validate_sync_heads_map(heads: dict) -> None:
+ for device_id, seq in heads.items():
+ if not isinstance(device_id, str) or not UUID_V4_LOWER_RE.fullmatch(device_id):
+ raise ValueError("invalid-device-id")
+ if not is_sync_head_seq(seq):
+ raise ValueError("invalid-head-seq")
+
+
def derive_sync_start_seq(heads: dict, device_id: str):
+ validate_sync_heads_map(heads)
if device_id not in heads:
return 0
head = heads[device_id]
- if not is_sync_head_seq(head):
- raise ValueError("invalid-head-seq")
if head == MAX_SAFE_INTEGER:
raise ValueError("sync-head-seq-overflow")
return head + 1
@@
def compare_sync_heads(left: dict, right: dict) -> str:
- for seq in list(left.values()) + list(right.values()):
- if not is_sync_head_seq(seq):
- raise ValueError("invalid-head-seq")
+ validate_sync_heads_map(left)
+ validate_sync_heads_map(right)
if set(left) != set(right):
return "divergent"
@@
def verify_sync_heads_disposition(vector: dict) -> None:
@@
for case in vector["invalid_head_seq_cases"]:
try:
derive_sync_start_seq(case["heads"], case["deviceId"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def derive_sync_start_seq(heads: dict, device_id: str): | |
| if device_id not in heads: | |
| return 0 | |
| head = heads[device_id] | |
| if not is_sync_head_seq(head): | |
| raise ValueError("invalid-head-seq") | |
| if head == MAX_SAFE_INTEGER: | |
| raise ValueError("sync-head-seq-overflow") | |
| return head + 1 | |
| def evaluate_sync_response_disposition(response: dict) -> str: | |
| return "request-next-page" if response["truncated"] else "complete" | |
| def compare_sync_heads(left: dict, right: dict) -> str: | |
| for seq in list(left.values()) + list(right.values()): | |
| if not is_sync_head_seq(seq): | |
| raise ValueError("invalid-head-seq") | |
| if set(left) != set(right): | |
| return "divergent" | |
| for device_id, seq in left.items(): | |
| if right[device_id] != seq: | |
| return "divergent" | |
| return "consistent" | |
| def verify_sync_heads_disposition(vector: dict) -> None: | |
| for case in vector["derive_start_seq_cases"]: | |
| assert derive_sync_start_seq(case["heads"], case["deviceId"]) == case["expected_start_seq"], case["name"] | |
| for case in vector["invalid_head_seq_cases"]: | |
| try: | |
| derive_sync_start_seq(case["heads"], case["deviceId"]) | |
| except ValueError as exc: | |
| assert str(exc) == case["expected_error"], case["name"] | |
| continue | |
| raise AssertionError(f"invalid head seq accepted: {case['name']}") | |
| for case in vector["derive_start_seq_overflow_cases"]: | |
| try: | |
| derive_sync_start_seq(case["heads"], case["deviceId"]) | |
| except ValueError as exc: | |
| assert str(exc) == case["expected_error"], case["name"] | |
| continue | |
| raise AssertionError(f"overflow case accepted: {case['name']}") | |
| for case in vector["response_truncation_cases"]: | |
| assert evaluate_sync_response_disposition(case["response"]) == case["expected_disposition"], case["name"] | |
| for case in vector["heads_comparison_cases"]: | |
| assert compare_sync_heads(case["left"], case["right"]) == case["expected_disposition"], case["name"] | |
| MAX_SAFE_INTEGER = (1 << 53) - 1 | |
| UUID_V4_LOWER_RE = re.compile( | |
| r"^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" | |
| ) | |
| def is_sync_head_seq(value) -> bool: | |
| return type(value) is int and 0 <= value <= MAX_SAFE_INTEGER | |
| def validate_sync_heads_map(heads: dict) -> None: | |
| for device_id, seq in heads.items(): | |
| if not isinstance(device_id, str) or not UUID_V4_LOWER_RE.fullmatch(device_id): | |
| raise ValueError("invalid-device-id") | |
| if not is_sync_head_seq(seq): | |
| raise ValueError("invalid-head-seq") | |
| def derive_sync_start_seq(heads: dict, device_id: str): | |
| validate_sync_heads_map(heads) | |
| if device_id not in heads: | |
| return 0 | |
| head = heads[device_id] | |
| if head == MAX_SAFE_INTEGER: | |
| raise ValueError("sync-head-seq-overflow") | |
| return head + 1 | |
| def evaluate_sync_response_disposition(response: dict) -> str: | |
| return "request-next-page" if response["truncated"] else "complete" | |
| def compare_sync_heads(left: dict, right: dict) -> str: | |
| validate_sync_heads_map(left) | |
| validate_sync_heads_map(right) | |
| if set(left) != set(right): | |
| return "divergent" | |
| for device_id, seq in left.items(): | |
| if right[device_id] != seq: | |
| return "divergent" | |
| return "consistent" | |
| def verify_sync_heads_disposition(vector: dict) -> None: | |
| for case in vector["derive_start_seq_cases"]: | |
| assert derive_sync_start_seq(case["heads"], case["deviceId"]) == case["expected_start_seq"], case["name"] | |
| for case in vector["invalid_head_seq_cases"]: | |
| try: | |
| derive_sync_start_seq(case["heads"], case["deviceId"]) | |
| except ValueError as exc: | |
| assert str(exc) == case["expected_error"], case["name"] | |
| continue | |
| raise AssertionError(f"invalid head seq accepted: {case['name']}") | |
| for case in vector["derive_start_seq_overflow_cases"]: | |
| try: | |
| derive_sync_start_seq(case["heads"], case["deviceId"]) | |
| except ValueError as exc: | |
| assert str(exc) == case["expected_error"], case["name"] | |
| continue | |
| raise AssertionError(f"overflow case accepted: {case['name']}") | |
| for case in vector["response_truncation_cases"]: | |
| assert evaluate_sync_response_disposition(case["response"]) == case["expected_disposition"], case["name"] | |
| for case in vector["heads_comparison_cases"]: | |
| assert compare_sync_heads(case["left"], case["right"]) == case["expected_disposition"], case["name"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/validate_test_vectors.py` around lines 306 - 358,
derive_sync_start_seq and compare_sync_heads must reject non-canonical deviceId
keys instead of accepting them; add a fast-fail check that validates the device
id format for the incoming device_id and for every key in the heads maps before
proceeding (use or add a helper like is_canonical_device_id or a regex check),
and raise a ValueError (e.g. "invalid-device-id") when any key is not canonical;
update derive_sync_start_seq to validate the single device_id and all keys of
heads, and update compare_sync_heads to validate all keys in left and right
prior to the existing seq checks/comparisons.
Comment on lines
+579
to
+694
| "derive_start_seq_cases": [ | ||
| { | ||
| "name": "missing-device-starts-at-zero", | ||
| "heads": {}, | ||
| "deviceId": "550e8400-e29b-41d4-a716-446655440000", | ||
| "expected_start_seq": 0 | ||
| }, | ||
| { | ||
| "name": "missing-device-in-non-empty-heads-starts-at-zero", | ||
| "heads": { | ||
| "660e8400-e29b-41d4-a716-446655440001": 9 | ||
| }, | ||
| "deviceId": "550e8400-e29b-41d4-a716-446655440000", | ||
| "expected_start_seq": 0 | ||
| }, | ||
| { | ||
| "name": "known-device-at-seq-zero-starts-at-one", | ||
| "heads": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 0 | ||
| }, | ||
| "deviceId": "550e8400-e29b-41d4-a716-446655440000", | ||
| "expected_start_seq": 1 | ||
| }, | ||
| { | ||
| "name": "known-device-at-seq-n-starts-at-n-plus-one", | ||
| "heads": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 41, | ||
| "660e8400-e29b-41d4-a716-446655440001": 9 | ||
| }, | ||
| "deviceId": "550e8400-e29b-41d4-a716-446655440000", | ||
| "expected_start_seq": 42 | ||
| } | ||
| ], | ||
| "invalid_head_seq_cases": [ | ||
| { | ||
| "name": "negative-seq-is-rejected", | ||
| "heads": { "550e8400-e29b-41d4-a716-446655440000": -1 }, | ||
| "deviceId": "550e8400-e29b-41d4-a716-446655440000", | ||
| "expected_error": "invalid-head-seq" | ||
| }, | ||
| { | ||
| "name": "fractional-seq-is-rejected", | ||
| "heads": { "550e8400-e29b-41d4-a716-446655440000": 1.5 }, | ||
| "deviceId": "550e8400-e29b-41d4-a716-446655440000", | ||
| "expected_error": "invalid-head-seq" | ||
| } | ||
| ], | ||
| "derive_start_seq_overflow_cases": [ | ||
| { | ||
| "name": "max-safe-integer-overflow-guard", | ||
| "heads": { "550e8400-e29b-41d4-a716-446655440000": 9007199254740991 }, | ||
| "deviceId": "550e8400-e29b-41d4-a716-446655440000", | ||
| "expected_error": "sync-head-seq-overflow", | ||
| "note": "Deriving start seq N+1 from Number.MAX_SAFE_INTEGER would leave the JS safe-integer range and is therefore rejected. The seq value itself is a valid JSON integer; only the N+1 derivation is guarded." | ||
| } | ||
| ], | ||
| "response_truncation_cases": [ | ||
| { | ||
| "name": "truncated-true-requires-another-request", | ||
| "response": { "truncated": true }, | ||
| "expected_disposition": "request-next-page" | ||
| }, | ||
| { | ||
| "name": "truncated-false-is-complete", | ||
| "response": { "truncated": false }, | ||
| "expected_disposition": "complete" | ||
| } | ||
| ], | ||
| "heads_comparison_cases": [ | ||
| { | ||
| "name": "identical-heads-are-consistent", | ||
| "left": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 0, | ||
| "660e8400-e29b-41d4-a716-446655440001": 9 | ||
| }, | ||
| "right": { | ||
| "660e8400-e29b-41d4-a716-446655440001": 9, | ||
| "550e8400-e29b-41d4-a716-446655440000": 0 | ||
| }, | ||
| "expected_disposition": "consistent" | ||
| }, | ||
| { | ||
| "name": "different-seq-values-are-divergent", | ||
| "left": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 4, | ||
| "660e8400-e29b-41d4-a716-446655440001": 9 | ||
| }, | ||
| "right": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 5, | ||
| "660e8400-e29b-41d4-a716-446655440001": 9 | ||
| }, | ||
| "expected_disposition": "divergent" | ||
| }, | ||
| { | ||
| "name": "missing-device-key-on-right-is-divergent", | ||
| "left": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 4, | ||
| "660e8400-e29b-41d4-a716-446655440001": 0 | ||
| }, | ||
| "right": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 4 | ||
| }, | ||
| "expected_disposition": "divergent" | ||
| }, | ||
| { | ||
| "name": "extra-device-key-on-right-is-divergent", | ||
| "left": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 4 | ||
| }, | ||
| "right": { | ||
| "550e8400-e29b-41d4-a716-446655440000": 4, | ||
| "660e8400-e29b-41d4-a716-446655440001": 0 | ||
| }, | ||
| "expected_disposition": "divergent" | ||
| } | ||
| ] |
There was a problem hiding this comment.
Add explicit invalid deviceId key coverage in heads maps.
This section still doesn’t assert disposition for non-canonical/non-UUID head keys. That leaves the key-shape rule unpinned by vectors and can reintroduce interop ambiguity.
💡 Suggested vector additions
"invalid_head_seq_cases": [
@@
{
"name": "fractional-seq-is-rejected",
"heads": { "550e8400-e29b-41d4-a716-446655440000": 1.5 },
"deviceId": "550e8400-e29b-41d4-a716-446655440000",
"expected_error": "invalid-head-seq"
+ },
+ {
+ "name": "non-uuid-device-key-is-rejected",
+ "heads": { "opaque-device-key": 3 },
+ "deviceId": "550e8400-e29b-41d4-a716-446655440000",
+ "expected_error": "invalid-device-id"
+ },
+ {
+ "name": "uppercase-uuid-device-key-is-rejected",
+ "heads": { "550E8400-E29B-41D4-A716-446655440000": 3 },
+ "deviceId": "550e8400-e29b-41d4-a716-446655440000",
+ "expected_error": "invalid-device-id"
}
],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-vectors/phase-1-interop.json` around lines 579 - 694, Add explicit test
vectors that cover non-canonical / non-UUID head keys: insert cases into
"derive_start_seq_cases" (e.g., heads with a non-UUID string key and an
empty-string key) that assert the same start-seq behavior or an
"invalid-head-key" error as appropriate; add cases to "invalid_head_seq_cases"
(or create a new "invalid_head_key_cases" array) with heads containing numeric
keys, empty string keys, and malformed UUID-like keys to assert the correct
rejection error (e.g., "invalid-head-key"); and add corresponding entries to
"heads_comparison_cases" where one side contains an invalid key to assert the
expected_disposition (e.g., "divergent" or an error disposition). Ensure each
new case uses the same naming pattern (e.g., "invalid-device-key-empty-string",
"invalid-device-key-non-uuid", "invalid-device-key-numeric") so they are
discoverable and cover the key-shape rule explicitly.
Contributor
Author
Loop-Review für PR #75Loop-Status: human-decision-required (Reviewer-Codex hatte ein medium-Finding zur non-UUID-deviceId-Carve-out-Formulierung in den Notes) Meine Findings (über den Loop hinaus)🔴 Blocker
🟡 Should-Fix
🟢 FYI / Nice-to-Have
Empfehlung
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/validate_test_vectors.py (1)
306-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStill missing:
deviceIdkey format validation in sync heads helpers.The validation helpers
derive_sync_start_seqandcompare_sync_headsstill accept arbitrary string keys in heads maps without enforcing the canonical lowercase UUID-v4 format required by Sync prose. This gap was flagged in a previous review and remains unaddressed. Without format validation, the script cannot properly verify test vectors that assert rejection of non-canonical deviceId keys (which are also missing from the JSON, per separate comment).🔧 Recommended fix
Add a validation helper and call it in the existing functions:
MAX_SAFE_INTEGER = (1 << 53) - 1 +UUID_V4_LOWER_RE = re.compile( + r"^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" +) def is_sync_head_seq(value) -> bool: # Phase 1 head seq values are non-negative JSON integers within the JS safe-integer range. # JSON booleans MUST NOT be treated as integers; fractional and negative values are rejected. return type(value) is int and 0 <= value <= MAX_SAFE_INTEGER +def validate_sync_heads_map(heads: dict) -> None: + """Validate that all deviceId keys in a heads map are canonical lowercase UUID-v4.""" + for device_id, seq in heads.items(): + if not isinstance(device_id, str) or not UUID_V4_LOWER_RE.fullmatch(device_id): + raise ValueError("invalid-device-id") + if not is_sync_head_seq(seq): + raise ValueError("invalid-head-seq") + + def derive_sync_start_seq(heads: dict, device_id: str): + validate_sync_heads_map(heads) + if not isinstance(device_id, str) or not UUID_V4_LOWER_RE.fullmatch(device_id): + raise ValueError("invalid-device-id") if device_id not in heads: return 0 head = heads[device_id] - if not is_sync_head_seq(head): - raise ValueError("invalid-head-seq") if head == MAX_SAFE_INTEGER: raise ValueError("sync-head-seq-overflow") return head + 1 def evaluate_sync_response_disposition(response: dict) -> str: return "request-next-page" if response["truncated"] else "complete" def compare_sync_heads(left: dict, right: dict) -> str: - for seq in list(left.values()) + list(right.values()): - if not is_sync_head_seq(seq): - raise ValueError("invalid-head-seq") + validate_sync_heads_map(left) + validate_sync_heads_map(right) if set(left) != set(right): return "divergent"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate_test_vectors.py` around lines 306 - 331, Add validation for deviceId key format (canonical lowercase UUID-v4) and call it from the existing sync-head helpers: implement a helper like is_valid_device_id(key) that verifies the canonical lowercase UUID-v4 regex, then in derive_sync_start_seq validate the incoming device_id with is_valid_device_id before looking it up, and in compare_sync_heads validate every key in both left and right (in addition to existing is_sync_head_seq checks) and raise ValueError (e.g., "invalid-device-id") for any non-conforming key so the functions enforce the required deviceId format.test-vectors/phase-1-interop.json (1)
579-694:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStill missing: test vectors for invalid
deviceIdkey formats.This section still lacks test cases asserting rejection of non-canonical
deviceIdkeys in heads maps, despite the previous review flagging this gap. Per the cited Sync prose,deviceIdkeys MUST be canonical lowercase UUID-v4 format. Without vectors covering invalid formats (non-UUID strings, uppercase UUIDs, empty strings, numeric keys, malformed UUIDs), implementations may diverge on whether they accept or reject such keys, creating interop ambiguity.🔍 Recommended test vector additions
Add cases to
invalid_head_seq_cases(or create a newinvalid_device_key_casesarray) that assert error disposition for non-canonical keys:"invalid_head_seq_cases": [ { "name": "negative-seq-is-rejected", "heads": { "550e8400-e29b-41d4-a716-446655440000": -1 }, "deviceId": "550e8400-e29b-41d4-a716-446655440000", "expected_error": "invalid-head-seq" }, { "name": "fractional-seq-is-rejected", "heads": { "550e8400-e29b-41d4-a716-446655440000": 1.5 }, "deviceId": "550e8400-e29b-41d4-a716-446655440000", "expected_error": "invalid-head-seq" + }, + { + "name": "non-uuid-device-key-rejected", + "heads": { "opaque-key": 3 }, + "deviceId": "550e8400-e29b-41d4-a716-446655440000", + "expected_error": "invalid-device-id" + }, + { + "name": "uppercase-uuid-device-key-rejected", + "heads": { "550E8400-E29B-41D4-A716-446655440000": 3 }, + "deviceId": "550e8400-e29b-41d4-a716-446655440000", + "expected_error": "invalid-device-id" + }, + { + "name": "empty-string-device-key-rejected", + "heads": { "": 3 }, + "deviceId": "550e8400-e29b-41d4-a716-446655440000", + "expected_error": "invalid-device-id" } ],Also add corresponding cases to
heads_comparison_caseswhere one side contains an invalid key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-vectors/phase-1-interop.json` around lines 579 - 694, The test vectors are missing cases asserting rejection of non-canonical deviceId keys; add a new array (e.g., "invalid_device_key_cases") or extend "invalid_head_seq_cases" with entries that use invalid keys (uppercase UUIDs, non-UUID strings, empty string, numeric keys, malformed UUIDs) referencing the same fields used in the diff ("heads", "deviceId", "expected_error") and set expected_error to "invalid-device-id"; also add mirror cases in "heads_comparison_cases" where one side contains an invalid key and expected_disposition is "divergent" to ensure implementations reject/non-match non-canonical device keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@scripts/validate_test_vectors.py`:
- Around line 306-331: Add validation for deviceId key format (canonical
lowercase UUID-v4) and call it from the existing sync-head helpers: implement a
helper like is_valid_device_id(key) that verifies the canonical lowercase
UUID-v4 regex, then in derive_sync_start_seq validate the incoming device_id
with is_valid_device_id before looking it up, and in compare_sync_heads validate
every key in both left and right (in addition to existing is_sync_head_seq
checks) and raise ValueError (e.g., "invalid-device-id") for any non-conforming
key so the functions enforce the required deviceId format.
In `@test-vectors/phase-1-interop.json`:
- Around line 579-694: The test vectors are missing cases asserting rejection of
non-canonical deviceId keys; add a new array (e.g., "invalid_device_key_cases")
or extend "invalid_head_seq_cases" with entries that use invalid keys (uppercase
UUIDs, non-UUID strings, empty string, numeric keys, malformed UUIDs)
referencing the same fields used in the diff ("heads", "deviceId",
"expected_error") and set expected_error to "invalid-device-id"; also add mirror
cases in "heads_comparison_cases" where one side contains an invalid key and
expected_disposition is "divergent" to ensure implementations reject/non-match
non-canonical device keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b688ac1-b242-4d14-8ace-63cd7b155c70
📒 Files selected for processing (4)
conformance/manifest.jsonscripts/validate_test_vectors.pytest-vectors/README.mdtest-vectors/phase-1-interop.json
Contributor
Author
Loop-Review für PR #75 (zweite Runde, nach CodeRabbit-Full-Review)Loop-Status: human-decision-required (Reviewer-Codex Finding zur Carve-out-Wording wurde manuell in Loop-Finding ohne Resolution (neu durch CodeRabbit, nach erster eli-review)CodeRabbit fordert in zwei Duplicate-Comments:
Einordnung: Inhaltlich legitim, aber außerhalb des hier definierten PR-Scopes. Die Acceptance Criteria nennen explizit „opaque non-UUID key treatment if that remains intentionally prose-backed" als zulässige Variante — d.h. Phase-1-Vektoren mussten die deviceId-Key-Shape NICHT abdecken. Der Validator hat das Format auch vor diesem PR nicht erzwungen; die Lücke ist preexistierend, nicht durch diesen PR eingeführt. Sinnvoller Folge-PR im selben Lane ( Meine Findings (über den Loop hinaus)🔴 Blocker
🟡 Should-Fix
🟢 FYI / Nice-to-Have
Empfehlung
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Task
Spec Refs
Acceptance
sync_heads_dispositionsection totest-vectors/phase-1-interop.jsonif the cases can be derived directly from existing Sync 002/003 prose without inventing new behavior.deviceId-> start seq0, knowndeviceIdwithseq=N-> start seqN+1, invalid head seq rejection,Number.MAX_SAFE_INTEGERoverflow guard,truncated=true->request-next-page,truncated=false->complete, identical heads ->consistent, different values ->divergent, missing/extra device keys ->divergent, and opaque non-UUID key treatment if that remains intentionally prose-backed.conformance/manifest.json,scripts/validate_test_vectors.py, andtest-vectors/README.mdso the section is validated and assigned towot-sync@0.1.Setup Commands
Checks
Persistent Context Refs
TDD
Human Gates
main; keep it spec-only and do not stack it on unrelated draft PR Draft: Public-Signed Spaces #12.mainor any default branch.Residual Risk
Summary by CodeRabbit
Tests
Documentation