Skip to content

Commit 633db5f

Browse files
kurodo3[bot]claude
andcommitted
fix: address Copilot review — spec accuracy, fork guard, version normalisation
- Spec doc: correct three stale inaccuracies about arrow-ipc metadata key ordering (same corrections already applied to the Rust repo's copy): IPC intro no longer claims insertion order is preserved; key_reorder section now says byte-identical IPC blobs; risk section corrected to say arrow-ipc sorts keys alphabetically, not preserves insertion order - golden-sync-check: add job-level if guard so the job is skipped for PRs from forks (secrets not available to forked workflows) - release.yml: strip leading 'v' from the version input before tagging to prevent 'vv0.3.0' tags when operator includes the prefix Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent d57d90a commit 633db5f

3 files changed

Lines changed: 28 additions & 11 deletions

File tree

.github/workflows/ci.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,11 @@ jobs:
6666
6767
golden-sync-check:
6868
runs-on: ubuntu-latest
69+
# Secrets are not available to fork PRs, so skip the check there.
70+
# Internal PRs and pushes to main always run it.
71+
if: >
72+
github.event_name != 'pull_request' ||
73+
github.event.pull_request.head.repo.full_name == github.repository
6974
permissions:
7075
contents: read
7176
steps:

.github/workflows/release.yml

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,5 +32,9 @@ jobs:
3232
3333
- name: Tag and push release
3434
run: |
35-
git tag v${{ inputs.version }}
36-
git push origin v${{ inputs.version }}
35+
# Strip a leading 'v' if the operator included one (e.g. "v0.3.0" → "0.3.0"),
36+
# then always prefix with 'v' so the tag is exactly "v0.3.0".
37+
VERSION="${{ inputs.version }}"
38+
VERSION="${VERSION#v}"
39+
git tag "v${VERSION}"
40+
git push origin "v${VERSION}"

docs/metamorphic/specs/2026-06-18-golden-vector-parity-design.md

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,11 @@ Each entry in `vectors`:
4646
| `ipc_b64` | string | Base64-encoded Arrow IPC stream (schema + optional rows) |
4747
| `expected_hash` | string | Rust-authoritative hex-encoded hash digest |
4848

49-
Arrow IPC is used for `ipc_b64` because it captures the exact bytes — including metadata key
50-
insertion order — that were fed to the Rust hasher. This eliminates any risk of Python
51-
constructing subtly different Arrow data.
49+
Arrow IPC is used for `ipc_b64` because it provides a stable, self-contained encoding of the
50+
Arrow schema (including all metadata) that both Rust and Python can deserialize identically.
51+
Note: `arrow-ipc`'s `metadata_to_fb` sorts metadata keys alphabetically before FlatBuffers
52+
encoding (`ordered_keys.sort()` in convert.rs), so the IPC byte stream is deterministic
53+
regardless of HashMap insertion order at the producer side.
5254

5355
---
5456

@@ -67,8 +69,12 @@ constructing subtly different Arrow data.
6769
| `empty_metadata_invariant` | No metadata at all — tested with `include_metadata=false`; `expected_hash` must equal that of the same schema hashed with `include_metadata=true` | `false` |
6870

6971
The `key_reorder_canonical` / `key_reorder_shuffled` pair encodes the key-ordering determinism
70-
invariant directly in the fixture: two different IPC blobs (different insertion orders) map to
71-
the same `expected_hash`.
72+
invariant directly in the fixture. Because `arrow-ipc` sorts metadata keys alphabetically before
73+
FlatBuffers encoding, both vectors produce **byte-identical IPC blobs** — the insertion-order
74+
invariant is enforced at the IPC level, not the hasher level. Both vectors therefore share the
75+
same `ipc_b64` and the same `expected_hash`. The test verifies that the hasher also produces
76+
matching output when the live hasher is called directly on schemas built with different insertion
77+
orders.
7278

7379
The `empty_metadata_invariant` entry pins the empty-metadata fixed point: a schema with no
7480
metadata must produce the same hash regardless of `include_metadata`. Only one entry is needed
@@ -374,10 +380,12 @@ Tag push fires the existing `publish.yml` → pure-Python package published to P
374380

375381
## Risks
376382

377-
- **IPC metadata order:** Arrow IPC preserves key insertion order in its FlatBuffers encoding.
378-
This is load-bearing for the `key_reorder_*` vectors. If a future Arrow version changes this
379-
behaviour the vectors would need to be regenerated, but the fixture format itself remains
380-
valid.
383+
- **IPC metadata order:** Arrow IPC does **not** preserve key insertion order — `metadata_to_fb`
384+
in arrow-ipc sorts keys alphabetically before FlatBuffers encoding (`ordered_keys.sort()` in
385+
convert.rs). As a result, the `key_reorder_canonical` and `key_reorder_shuffled` vectors
386+
produce byte-identical IPC blobs. If a future Arrow version changes this sorting behaviour,
387+
the `key_reorder_*` IPC blobs would diverge and the vectors would need to be regenerated;
388+
the fixture format itself remains valid.
381389
- **Fixture drift:** Mitigated by the `golden-sync-check` CI job. If the GitHub App secret
382390
expires or is revoked, the drift check will fail loudly rather than silently passing.
383391
- **Version/tag sync:** The `verify-version-tag-sync` CI job enforces the invariant on every

0 commit comments

Comments
 (0)