refactor(headless): unify canonicalJson into single serializer (#1404) - #2005
Conversation
c709d42 to
44dce97
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Review: MERGE-READY ✅ (independent deepseek-v4-flash review)
Reviewed the full diff (+111/−129, 9 files) and verified serializer equivalence by direct execution.
- Byte-identity verified: old vs. new
canonicalJsonare byte-identical on nested objects, arrays, unicode,1.5e-300,9007199254740993,-0,NaN/Infinity(→null),Date, empty objects, symbol keys — with one divergence: the oldharness-oracle-registry.tscopy emitted invalid JSON for objects withundefined-valued fields ({"u":undefined}); the unified version drops the field. Traced all registry fingerprint inputs — every reachable input is all-defined, so the divergence is dormant today and internally consistent (both generation and verification now share one function). - Unification is complete: all five duplicate copies removed, three hand-rolled
sha256:…update(canonicalJson(…))wrappers collapsed intobuildRunManifestFingerprint; no dangling references to deleted functions;index.tssurface unchanged. - Drift sentinel works: the frozen verbatim port in
prompt-ab-fingerprints.test.tspasses in CI — independent evidence the unified serializer preserves legacy prompt fingerprint bytes. - Typecheck/lint/format green; headless-relevant checks all pass.
Non-blocking P3 (optional): (1) canonicalJson([undefined]) still yields invalid JSON (pre-existing, unreachable); (2) the registry divergence has no regression test; (3) docstring "every fingerprint must go through here" overstates (harness-oracle-policy has its own intentional fingerprintValue); (4) CI e2e red is an unrelated desktop plan-reminders flake — nothing in this PR touches outside packages/headless.
Merge prerequisite: the only red check (e2e / e2e_shard (1/2)) is an unrelated, pre-existing desktop flake — please re-run the e2e job (or confirm the flake's known-handling) before merging; the PR itself needs no changes.
|
Thanks for the thorough review and for verifying serializer equivalence by direct execution — really appreciated. Both of your non-blocking suggestions are addressed in the follow-up commit (ae6354d): Tightened the docstring (P3 #3) — it no longer claims "every fingerprint must go through here." It now makes explicit that harness-oracle-policy.ts keeps its own JSON.stringify-based fingerprintValue (for a single frozen as const policy object, deliberately not canonicalized), while resume/integrity fingerprints go through buildRunManifestFingerprint. Added a registry divergence regression test (P3 #2) — pins the previously divergent scenario: a registry-shaped payload with a nested undefined field (identity.evidenceFingerprint) now hashes identically to the all-defined payload. Before unification, the divergent local copy would have emitted invalid JSON ({"field":undefined}) and never matched. Left P3 #1 (canonicalJson([undefined])) untouched — it's pre-existing and currently unreachable, so it stays out of scope. |
|
It looks like only maintainer can re-run the CI. Please help me re-run CI. |
|
Could you rebase onto current main? The branch base predates a lot of recent work, and the e2e failure is the sidebar rename spec (sidebar-navigation.spec.ts:72): it races without the #2022 fix (2e30bd8, seed a real project for the rename spec), which landed on main after this branch forked. Not caused by this PR. After a rebase the failed jobs should go green, and the earlier approval still applies. |
…e#1404) Consolidate the six scattered `canonicalJson` copies in @maka/headless into a single authoritative implementation exported from ab-manifest.ts, next to buildRunManifestFingerprint. The divergent copy in harness-oracle-registry.ts was missing the `undefined`-field filter, producing invalid JSON for objects with undefined values; traced all call sites and confirmed no current input can contain undefined fields, so the fix is behavior-neutral today and removes a latent footgun. Also collapses the hand-rolled `sha256:...update(canonicalJson(...))` expressions in runtime-policy-ab-run.ts, kimi-protocol-ab.ts, and prompt-ab-manifest.ts into buildRunManifestFingerprint, and removes the duplicate `fingerprintValue` wrapper in harness-oracle-registry.ts. The frozen legacy oracle in prompt-ab-fingerprints.test.ts is intentionally left as an independent reference implementation — coupling it to the authoritative copy would defeat its purpose as a drift sentinel. Adds behavior tests (per apache#1403) pinning the canonicalJson and buildRunManifestFingerprint invariants: undefined filtering, key-order independence, array-order preservation, and fingerprint stability.
… regression (apache#1404) Address non-blocking review feedback: - Tighten the canonicalJson docstring: it no longer claims "every fingerprint must go through here" — harness-oracle-policy.ts keeps its own JSON.stringify-based fingerprintValue for the execution-policy fingerprint (a single frozen as-const object, deliberately not canonicalized). - Add a regression test pinning the harness-oracle-registry divergence scenario: a registry-shaped body with a nested undefined field must hash identically to the all-defined body. Before unification the divergent local copy omitted the undefined-field filter and would emit invalid JSON ({"field":undefined}), never matching the canonical computation.
ae6354d to
fc2331f
Compare
|
OK, as you said, all passed, thank you! |
#Summary
Single authoritative serializer. Promoted canonicalJson in ab-manifest.ts to export function (body unchanged), next to buildRunManifestFingerprint. This is the load-bearing determinism contract for @maka/headless — every A/B resume identity, integrity check, and equivalence comparison depends on it.
Removed 5 duplicate copies. Replaced the copies in prompt-ab-manifest.ts, ab-summary.ts, runtime-policy-ab-run.ts, kimi-protocol-ab.ts, and harness-oracle-registry.ts with imports. The copy in harness-oracle-registry.ts had diverged — missing the .filter(([, v]) => v !== undefined), producing invalid JSON for objects with undefined values. Traced all 6 call sites: inputs come from object literals, JSON.parse, or null (not undefined), so the fix is behavior-neutral today but closes a latent footgun.
Collapsed hand-rolled fingerprints. Folded the sha256:...update(canonicalJson(...)) expressions in runtime-policy-ab-run.ts, kimi-protocol-ab.ts, and prompt-ab-manifest.ts into buildRunManifestFingerprint, and removed the duplicate fingerprintValue wrapper in harness-oracle-registry.ts.
Added behavior tests. Pinned the invariants in ab-manifest.test.ts: undefined filtering, key-order independence, array-order preservation, recursive sorting, and fingerprint stability.
Intentionally left independent. The frozen legacy oracle in prompt-ab-fingerprints.test.ts stays standalone — it's a drift sentinel, and coupling it would defeat its purpose. fingerprintValue in harness-oracle-policy.ts (plain JSON.stringify) and the @maka/runtime / @maka/core serializers are different semantic families, out of scope.
Refs: #1404
Verification
npm run format:checknpm run lintnpm run typecheck (packages/headless)— passed, 0 errorsnpm --workspace @maka/headless run build— passednpm --workspace @maka/headless run test:dist--1309 passed, 5 skipped