Conversation
…laration (LAB-3967) verify() now hard-fails any python-frame vector declaring `twin_of` that differs from its base beyond envelope encoding (value_json, frame-prefix bytes, compressed_data_hex, checksum_hex, original_size, format, inner_msgpack_hex), or whose declaration cannot hold: unknown base, self-reference, same-encoding copy, envelope-less base, duplicate vector names. Until now that check ran only at generate time, as a warning. Keyed on the declaration, not the bytes: from the bytes alone "the wheel drifted" and "the protocol legitimately moved while the legacy vector stayed frozen" are indistinguishable, so mirroring the byte compare into CI would move the generator deadlock one level up. The exit for a legitimate protocol evolution is dropping `twin_of` in the same commit as the regenerated bytes — a reviewable fixture diff — with every byte comparison intact and both encodings still observed. - generate keeps warning (never raising) and names both exits - _upsert carries `twin_of` across rebuilds; generate never adds/drops it - the generator's `_bin` description no longer claims twin-ness in prose, so the exit survives the next generate - fixture: `twin_of` added to the `_bin` vector, description reworded; every frame/payload/envelope byte unchanged (sha256 d8a3756a… → f43eb733…); no repo in the org vendors this fixture - test_python_frame_reference.py rewritten as a stdlib mutation suite that runs verify against a mutated copy of the committed fixture - dead `import msgpack` in generate(); bare `assert` on module import in the test → explicit ImportError
…(LAB-3967) The diverged generate-time case built its envelope from the legacy vector, so it inherited envelope_encoding "int-array" and the new "a twin must differ from its base in encoding" rule fired before the field compare; the case then asserted the wrong warning text.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: cachekit-io/protocol/.coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
…FAIL, not a traceback (LAB-3967)
The presence guard covered value_json, expected_payload_hex and
payload_envelope, then strictly indexed frame_hex and the five envelope
fields the twin compare reads. A vector whose payload_envelope lacked
one of them made verify() die with a KeyError traceback instead of the
`FAIL {name}: ...` line the gate promises, and the generate-time warning
path raised where its contract is "warn, never raise".
The guard now covers every field the compare reads, and tests the value
rather than the key: a JSON null on both sides would otherwise compare
equal and pass the twin claim vacuously. A non-string `twin_of` (an
unhashable list, say) is reported the same way instead of a TypeError.
The comparison itself stays strict indexing after the precondition.
The mutation suite pins each case against the pre-fix behaviour; the
CHANGELOG sentence names partial-envelope vectors alongside envelope-less
ones.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@kody start-review |
1 participant
Summary
This PR promotes the twin-equivalence check for frame test vectors from a generate-time-only stderr warning into a hard
verifygate, driven by a new operator-ownedtwin_ofdeclaration (LAB-3967, follow-up to LAB-1203).Problem
The
_binvector intest-vectors/python-frame.jsonclaims to be an "encoding-only twin" of the legacydefault_saas_write_msgpack_bytestoragevector — differing only in envelope encoding (msgpackbinvs. array-of-integers). Previously that claim was:description), andgeneratetime as a stderr warning that CI never reads.The check could not be made a hard failure at generate time: the legacy array-of-ints wheel no longer ships in any installable release, so a legitimate future write-path change would permanently deadlock
generate(the twin can never re-match a frozen legacy vector).Changes
New
twin_offield. The_binvector now carries"twin_of": "default_saas_write_msgpack_bytestorage"— an explicit, operator-owned declaration living in the fixture rather than in code. From the bytes alone, "the wheel drifted" and "the protocol legitimately moved" are indistinguishable, so the claim must be declared rather than inferred.verifybecomes the CI gate. A shared_twin_divergence()helper hard-fails any declared twin that differs from its base beyond envelope encoding, checkingvalue_json, the frame prefix (compared as raw bytes, so a header key reorder is caught), and envelope fieldscompressed_data_hex,checksum_hex,original_size,format, andinner_msgpack_hex. It also fails vacuous declarations — self-reference, same-encoding copies, envelope-less bases, unknown vector names, and duplicate vector names.generatekeeps warning, never raising. The old_require_twin_equivalencebecomes_warn_twin_divergence, sharing the same divergence logic asverifyso the two can't disagree on what "twin" means. The warning now names the two legitimate exits: fix the wheel/codec, or droptwin_ofin the same commit as the regenerated bytes._upsertcarriestwin_ofacross rebuilds. The field is preserved on rewrite (and ignored when comparing for no-op churn), and is never added or dropped automatically — that stays the operator's reviewable move. The_bindescription is moved to aBIN_DESCRIPTIONconstant that no longer makes the twin claim in prose (the field is the claim), so the gate survives the nextgenerate.Tests.
tools/test_python_frame_reference.pyis rewritten as a mutation suite over the committed fixture, provingverifyfails on each covered field, that droppingtwin_ofstays green without loosening any byte compare, thatgeneratewarns but never raises (the LAB-1203 deadlock pin), and that_upsertcarries the declaration correctly.Impact
The fixture sha256 changes because of JSON metadata only (
twin_ofadded,_bindescription reworded); everyframe_hex,expected_payload_hex,expected_header, andpayload_envelopebyte is unchanged. No SDK vendored this fixture at the time, so nothing downstream needs to re-pin. Documentation inspec/wire-format.mdandCHANGELOG.mdis updated accordingly.Summary
This PR hardens the twin-equivalence verification gate to fail gracefully on malformed or incomplete twin declarations, ensuring these produce clean
FAILoutput lines instead of unhandled tracebacks.Changes
Robustness of the
_twin_divergencecheck (tools/python-frame-reference.py):twin_ofis a string vector-name before use; a non-string value now yields a descriptive failure reason rather than crashing.nullvalues as missing, and addsframe_hexto the list of required fields.payload_envelopesub-fields, so a partial envelope (missing or null sub-field likeinner_msgpack_hex) is reported by name.Test coverage (
tools/test_python_frame_reference.py):nullenvelope sub-field on both sides, and a non-stringtwin_ofeach causeverifyto exit1with a namedFAILline rather than a traceback.generate-path case where the base vector lacksframe_hex, confirming it warns without raising.Documentation (
CHANGELOG.md):FAILline rather than a traceback.Purpose
The core intent is to ensure the twin-equivalence gate can never crash on malformed input — every invalid twin declaration produces a diagnostic failure message naming the specific problem, and
nullvalues are prevented from vacuously satisfying the equivalence claim.