Make JSON object key order and string escaping byte-identical to Go (#183) - #185
Merged
Conversation
Go's encoding/json sorts map keys; pine-java emitted insertion order, so
`{"c10":..,"c2":..,"c1":..}` came out unsorted where Go gives c1, c10, c2.
Values were identical — only the order differed — but /execute output is a
byte-exact contract.
Two distinctions decide the implementation, and getting either wrong produces a
fix that looks right on ASCII fixtures.
**Maps sort, structs do not.** pine-go's response envelope is a struct
(`executeResponse`: common, items, warnings, trace, error) while the payloads
inside it are `map[string]any`. So the envelope keeps declaration order and
everything within it sorts. My first attempt registered a serializer for every
Map, which moved "error" ahead of "items" and broke the partial-error byte-exact
fixture — caught by cross-validate section 14 dropping from 55 to 54. Java has no
struct/map distinction to key off, so it is now explicit: payloads are wrapped in
GoFormat.SortedByUtf8, the envelope is a plain LinkedHashMap.
**UTF-8 bytes, not String.compareTo.** Jackson offers
ORDER_MAP_ENTRIES_BY_KEYS, but it sorts by natural ordering, i.e. UTF-16 code
units. Go compares Go strings, i.e. UTF-8 bytes. These disagree above the BMP,
because a non-BMP character is a surrogate pair starting 0xD800 in UTF-16 but a
4-byte sequence starting 0xF0 in UTF-8:
U+FFFD UTF-16 fffd UTF-8 ef bf bd
U+10000 UTF-16 d800 dc00 UTF-8 f0 90 80 80
so compareTo puts U+10000 first while Go puts U+FFFD first. Using the built-in
feature would have relocated the divergence from ASCII keys to emoji keys rather
than removing it. Verified against Go: json.Marshal emits
{"z":3,"�":1,"\U00010000":2}, which is what this now produces.
Also removes PineServer's local sortMapKeys/sortItemKeys/sortListElements. Those
were a partial fix on the HTTP path only — which is why RunCli diverged while the
server did not — and they used TreeMap, so they carried the UTF-16 bug. One
comparator at the serializer now covers both entry points.
pine-cpp needed no change: `std::string` comparison is byte order, so it already
matched Go, including above the BMP. The issue title and this task's framing both
assumed two runtimes were wrong; only one was.
Seven new tests, each verified by mutation: swapping compareUtf8 for
String::compareTo reddens the non-BMP case, and dropping the nested wrap reddens
the depth case. 343 Java tests, 247 C++ cases, cross-validate 55/55,
differential-fuzz 1000/1000.
Before this, nothing could catch #183 — which is why it survived so long. Three independent blind spots, all in the gates rather than the code: **Section 9 called itself "no normalization" and had a normalized fallback.** On a byte mismatch it re-compared through normalize_json and counted equality as a pass, emitting only a `[W] key ordering differs` line. That fallback existed precisely to tolerate this divergence, so the section named for byte parity could not detect the one byte difference it was papering over. Removed; a byte mismatch now fails. Verified: 91/91 both pairs with the fix, and immediately red when the Java serializer is reverted. **differential-fuzz compared after sort_keys=True.** Key order was erased before comparison. Added key_order_signature, which reads order out of the raw text via object_pairs_hook, so ordering is checked separately while the value comparison keeps the float tolerance and item-order normalization it needs. **The fuzz generator emitted sorted flow_contracts.** This one is the most instructive: even with the ordering check added, 1000/1000 still passed against a deliberately broken serializer. The generator built contracts with `sorted(common_outputs)`, so declaration order always already equalled sorted order and insertion-order output coincided with Go's. A generator that only produces inputs already in the expected shape cannot detect a bug about shape. Contracts are now shuffled, and the mutant fails 6 of 60 rounds. I also had the new check gated on `strict_order` at first, reasoning that key order only matters when item order is deterministic. Wrong, and it silently disabled the check for most rounds since strict_order is only true when the pipeline ends in a sort. Object key order is deterministic regardless of item order; the two are independent. When item order is non-deterministic the per-item key sequences are compared as a multiset, so each item's own key order is still checked exactly. cross-validate 55/55, differential-fuzz 1000/1000 with all three gates active.
…el table Reflection: memory/reflections/json-key-order-parity-183.md New reference/json-key-order-parity.md rather than folding into number-formatting-parity.md: both describe the same serialization path, but one is about how a number is spelled and the other about what order keys come in — mixing them would blunt both lookups. Cross-pointers added in three places. The two rules are the ones I got wrong on the first attempt: - Go sorts map keys but leaves struct fields in declaration order, and pine-go's response envelope is a struct while the payloads inside it are maps. Registering a sorting serializer for Map.class — the obvious move — reorders the envelope and breaks 02_partial_error_keeps_partial_result.json, which is how cross-validate went 55 to 54. - The sort key is UTF-8 bytes. Jackson's ORDER_MAP_ENTRIES_BY_KEYS and TreeMap both use String.compareTo, i.e. UTF-16 code units, which disagrees above the BMP and would have moved the divergence from ASCII keys to emoji keys. Also states plainly that pine-cpp needs nothing: std::string comparison is byte order, so it already matched Go including above the BMP. ci-quality-baseline.md's channel-visibility table was stale in three places and is rewritten: 09-raw-byte.sh no longer falls back to normalized comparison so it is a real byte channel now (14 is no longer the only one), differential-fuzz gained key_order_signature so key order is no longer structurally invisible, and the generator shuffles contracts. The number-visibility rows are unchanged. Two new disciplines, both learned the hard way this round: - A generator that only emits inputs already in the expected shape cannot detect a bug about shape. After adding the ordering check the fuzz still passed 1000/1000 against a deliberately broken serializer, because every generated flow_contract was sorted. The criterion is to verify a new check goes RED against a mutant, not merely green against correct code. - A new gate condition needs its true-rate measured. I gated the ordering check on strict_order, which is only true when a pipeline ends in a sort, so it was off for most rounds — and key order is independent of item order anyway. doc-gaps.md gains a closed-items section: #183 moves there, keeping the answer to what the old entry listed as unknown (pine-cpp was already correct). The byte-channel entry stays open with (b) done and (a), widening fixtures/server_byte_exact/, still outstanding — no fixture was added this round.
BLOCKING, from the independent review: my key-order fix covered /execute's common and items but missed the trace snapshots in the same response. Go's traceEntry.InputSnapshot and OutputSnapshot are map[string]any (server.go:667-668) so they sort, while Java emitted them in insertion order. output_snapshot.item_writes is worse: its Go type is map[int]map[string]any, and encoding/json renders int keys as strings and sorts those, so item index 10 belongs between 1 and 2, not after 9. The reviewer also warned that wrapPayload would throw on that map, since its Java keys are Integer — confirmed, so wrapping now goes through withStringKeys rather than a cast. The trace ENTRY itself stays unwrapped: it is a struct in Go, so its own field order is declaration order. Every gate was blind to this, which is why my own verification missed it: 09 and differential-fuzz drive the CLI, whose output has no trace at all; 14's fixtures set no _return_trace; and 06 printed sorted(trace[0].keys()) before comparing, discarding the very dimension under test. Section 06 now compares the key sequence and nested shape with object_pairs_hook, enables debug so snapshots are actually present, and pads the request past 10 items with unsorted extra keys — without that padding the check passed even against a deliberately broken serializer, because a one-key snapshot has no order to get wrong. IMPORTANT: /stats diverged too, and this one needed both runtimes. Go builds that response as map[string]any (server.go:760) so its top level sorts, unlike /execute's struct envelope. But the branches beneath mix the two rules — serverStats() is a map[string]int64 and sorts, while SchedulerStatsSnapshot is a struct and must not — so it is wrapped branch by branch, per the Go type. My first attempt wrapped the whole tree and reordered the scheduler struct. pine-cpp hand-builds this JSON by concatenation, so it emitted source order at the top level AND inside server. Adding the gate is what exposed it; the issue title said Java only, and for /execute that was right. Now both collect members into a std::map, whose byte-order iteration is Go's sort order. IMPORTANT: sendResponse used writeValueAsBytes, which escapes characters above the BMP as surrogate pairs, while Go and C++ emit raw UTF-8. So the server diverged on any emoji in a key or value, and the new unit tests could not see it because they assert against writeValueAsString — a different encoding path from the one production used. Now writeValueAsString(...).getBytes(UTF_8). Two minors: an orphaned javadoc left by the deleted TreeMap helpers, which described a method that no longer exists and called the rule "alphabetically" — the spelling this task established as wrong for anything outside ASCII; and SortedByUtf8's javadoc had been inserted between createGoCompatMapper's javadoc and its method, orphaning the latter. 343 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55 (section 06 now 22/22 both pairs), differential-fuzz 1000/1000.
The review's blocking finding invalidated two things I had written a commit earlier, so this corrects them rather than leaving the reference describing a narrower fix than shipped. "pine-cpp needs no change" was true only of the path I had looked at. Responses serialized through the Variant writer sort by byte order for free, but /stats is hand-built by string concatenation in server.cpp and emitted source order at the top level and inside `server`. The reference now says which code path the "natural" guarantee applies to, and records the lesson: "this runtime already satisfies it" is a claim about a path, not about a runtime. The issue title and the task description both said Java only — correct for /execute, wrong for /stats, and only the new gate surfaced it. Added a coverage table naming each response position, its Go type, which of the two rules applies, and that it is now aligned. /stats has both rules in one tree, which is why the Java side wraps branch by branch and why sortedShallow exists; a single blanket wrap sorts the scheduler struct that Go leaves alone. The channel list now records that 09-raw-byte and differential-fuzz drive the CLI and therefore cannot see trace or /stats at all, and that 06-server-http needs all three of its new conditions — debug enabled, 12 items, extra unsorted common keys — because a one-key snapshot has no order to get wrong and fewer than 10 items cannot detect string-sorted integer keys. ci-quality-baseline.md gains the general form: three channels were blind to the same place, which was not coincidence but the result of never asking which channel can see the field a contract lives on. That question now precedes the question of what a channel normalizes.
…fixture BLOCKING, from the second review: pine-cpp still emitted /stats.operators in pipeline declaration order. Go's Stats() returns map[string]OpStatsSnapshot (pine.go:285) inside a map response, so encoding/json sorts the operator names, while Stats::snapshot() returns a vector "ordered by the pre-init sequence" (server.hpp:78). Measured across three live servers: Go and Java give lua_discount, lua_skip_cheap, lua_stats, recall_items, sort_by_price; C++ gave recall_items, lua_discount, lua_stats, lua_skip_cheap, sort_by_price. Reachable on any GET /stats whose operator names are not already alphabetical, which is 30 of the tracked fixtures. The part worth recording is why my own new gate missed it. Both /stats checks drive fixtures/pipelines/transform_then_filter.json, whose operators are copy_score and truncate — already in alphabetical order, so an engine emitting pipeline order produced identical bytes. That is precisely the failure mode the doc section I wrote one commit earlier describes: an input already in the expected shape cannot detect a bug about shape. I had applied it to the fuzz generator and missed it in my own fixture choice. The section now renames operators into reverse-alphabetical order before starting the servers, and goes red against the unsorted C++ implementation. Also from the same review: two of that gate's three new conditions do not carry weight. The *_probe common keys never reach input_snapshot, because snapshotInput only emits an operator's DECLARED common_input — and declaring them there breaks transform_copy, whose metadata arity must match its output list, panicking all three runtimes. Only the 12-item padding has teeth, via output_snapshot.item_writes. Rather than leave the claim overstated, the script now records what each condition does and does not pin, and three unit tests cover what the channel cannot: the trace entry keeping declaration order while its snapshots sort, integer keys sorting as strings, and sortedShallow not descending. Each verified red against the corresponding revert. Docs corrected where they still asserted what the previous commit retracted: index.md and doc-gaps.md said pine-cpp needed no change, and the reference coverage table named every /stats position except the one still broken — which made it read as complete. The reference now records that the first version of the gate missed operators for a whole round because of the fixture's names. 346 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55 (section 06 now 22/22 both pairs), differential-fuzz 1000/1000.
…claration The third full-range review returned APPROVE with zero findings. It named one gap without counting it: input_snapshot key ordering was pinned only by a unit test, since the section-06 fixture cannot reach it — snapshotInput emits only an operator's declared common_input, and transform_copy's arity forbids adding keys there. New check [14c] uses a second fixture whose operator declares two common_input fields, so the snapshot has an order to get wrong. It goes red when input_snapshot wrapping is reverted, which nothing did before. Adding it reproduced this task's recurring mistake a third time, which is the part worth recording. control_op_nil_field_no_crash's ctrl_if declares ["event", "expose_duration"] — already alphabetical, so an engine emitting insertion order produced identical bytes and the new check passed against the very defect it was written for. The check now reverses each declared common_input first. Three instances now, in three different forms: the fuzz generator emitting sorted() flow_contracts, section 06's operator names being copy_score/truncate, and this fixture's field declaration. Same mechanism every time — the input was already in the expected shape, so the check was green against a broken implementation. The reference now states the rule directly: when writing a key-order check, first confirm the fixture's declaration order differs from sorted order, and verify the check goes red against a mutant, because green is not evidence. 346 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55 (section 06 now 23/23 Go-vs-Java), differential-fuzz 1000/1000.
IMPORTANT, from the fourth review: the reflection still stated "本次没有改 pine-cpp" and index.md copied that conclusion into its summary, while this range changed server.cpp by +57/-18 across three /stats fixes. Commit b1cfebd set out to retract exactly that claim and updated the reference and the guide but never went back to the reflection — so one commit contained the correction and its own contradiction, and the index pointed readers at the wrong half. A reader routing by that summary would conclude pine-cpp needed no attention and skip the only place recording the hand-written-JSON risk. The reflection also had zero mentions of /stats, which is the most instructive part of this task, and its test count was 343 (new 7) against an actual 346 (new 10). Both corrected, plus a new section on why /stats was missed twice — once because I read the issue title as naming the endpoint rather than the contract, and once because the gate fixture's operator names were already alphabetical. Two minors, both real: - 14c used a fixed `sleep 4` where the rest of the section polls srv_ready. Under set -euo pipefail an unready server makes curl return empty, python3 exit 1, and pipefail abort the section at that line — every later check silently skipped with no red printed. Now polls, and a parse failure surfaces as a failure instead of a truncation. Also records why 14c compares Go and Java only: C++'s snapshot_input omits null values, a pre-existing value difference that would fail this check for the wrong reason. - The writeValueAsBytes to writeValueAsString fix had no gate at all. No fixture in fixtures/server_byte_exact/ contained a single non-ASCII character, and both other byte channels drive the CLI, which does not use that code path. New fixture 07_non_bmp_keys uses keys whose UTF-16 and UTF-8 orders disagree, so it pins both the escaping fix and the comparator: reverting either turns it red, verified separately. 346 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55 (section 06 23/23, section 14 now 6/6 both pairs), differential-fuzz 1000/1000.
Fifth review: 0 blocking, and all three findings are documentation claims that no longer match the code — two of them invalidated by 81f3ed1, the last commit of the round before. - doc-gaps.md said section 14 has "只有 5 个 fixture" and that #183 added none. There are 6; 81f3ed1 added 07_non_bmp_keys.json. The entry stays open, because 6 fixtures still cover far less than a global byte-exactness claim implies, but it now says what exists and what the new one pins. - The reflection header described this as "两个 commit" against an 8-commit range, and its Follow-up section repeated the no-new-fixture claim. Both corrected. - 06-server-http.sh had two comments contradicting each other about whether the probe keys reach input_snapshot: an inline one saying they do, and a NOTE block saying they do not. The NOTE is right — verified again — so the inline comment is now consistent with it. Worth naming the pattern: this is the third round whose findings were all of the form "a claim written before the last change, not revisited after it." The reflection has now been corrected twice for exactly this. The durable lesson is that a doc edited mid-task needs re-reading at the end of the task, not at the end of the commit that touched it — recorded in the reflection itself. 346 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
Sixth review: 0 blocking. The most useful finding is a comment of mine that states the wrong Go type, and currently works only by accident. handleStats justified deep-wrapping `http` with "map (nested maps)". Its innermost values are HttpDurationBucket STRUCTS (http_metrics.go:56), so Go keeps their fields in declaration order while GoFormat.sorted descends and sorts them. The output matches today only because that struct has exactly two fields whose declaration order already equals their UTF-8 sorted order — count before sum_ns. A third field added out of alphabetical order would diverge silently, which is the same trap the block already warns about for `scheduler`, one level deeper. The comment now says so, and a test asserts the coincidence explicitly so that breaking it fails there instead of at runtime. index.md:42 still said 5 fixtures and that pine-cpp needed no change. 09c508c's message claims it corrected index.md and doc-gaps.md on that point; it corrected line 30 only, and line 42 then survived two further doc-correction commits. Both halves fixed, along with a channel count that said three where the reference lists four. The reflection's commit count is the interesting one: it said 8 against a range of 9, because the commit that rewrote the line to fix a stale count computed it before itself and was therefore stale on arrival. Rather than write a third number that will expire the same way, the line now says the count is not a thing to record and points at git rev-list. That is the general fix for this round's whole finding class — four consecutive rounds found only claims written before a later change and never revisited, twice in this same file. Note on this round's evidence: the reviewer's snapshot was deleted mid-review, so it performed live three-way probes and input analysis but no mutation testing, and could not run sections 09/14 or the Java suite. It reported that limit plainly. Those checks were exercised by rounds 3 through 5 at earlier heads; recorded in the run manifest rather than papered over. 347 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
Seventh review: 0 blocking, 0 important. Both minors are the same stale-claim class, and both are now fixed at the root rather than by writing a fresh number. dag-engine.md still directed readers to "why pine-cpp naturally satisfies it". That file was written by 87c1e2a and never revisited, while 88ed692 and 09c508c changed pine-cpp/src/server/server.cpp in the same range. So it contradicted index.md, the reference, and doc-gaps.md — and stood as a live counter-example to the reference's own first lesson, that "this runtime satisfies it naturally" is a claim about a code path and not about a runtime. It now points at which path satisfies it and which does not. The reflection's test count said 346 / 10 new against an actual 347 / 11. The eleventh test was added by the very commit that edited that file, whose message stated 347 correctly — the number was known and simply not carried across. Rather than write 347 and wait for it to expire, that line now says the count is not a thing to record, matching what the commit-count line already does. This round also delivered what round 6 lost to the snapshot cleanup: 18 mutations, each first shown to change semantics, covering the comparator, the wrapper registration, shallow-versus-deep wrapping, both trace snapshots, the response byte-encoding path, and all three C++ /stats sort sites. Every gate went red where it should. All six of the docs' NEGATIVE claims also held under mutation — the probe keys are inert, only the 12-item padding carries weight, debug is necessary but not sufficient, and the operator rename, the 14c common_input reversal and the fuzz shuffle are each individually load-bearing. 347 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
BLOCKING, from the eighth review. pine-cpp applied Go's string escaping to JSON values but not to object keys: values went through detail::write_go_string, which emits the HTML-safe forms for < > & and escapes U+2028/U+2029, while all three w.Key() sites handed the key straight to RapidJSON, which applies none of them. So the same character diverged in a key while matching in a value. Reproduced on both the CLI and /execute — Go and Java emit "a<b", C++ emitted raw "a<b", and U+2028 came out as bare UTF-8 against Go's . Pre-existing rather than introduced here, but squarely a counter-example to the contract this range documents: the reference says a response serialized through the Variant writer needs no attention, and the coverage table marks /execute's common and items aligned. And it was invisible to every channel — no fixture in the repo had such a key, and the fuzzer's field pools are [a-z_] only. The existing html_chars_passthrough.json fixture covers the value side alone. The mechanism is worth naming, because it is a new shape of this task's recurring problem: one string property (escaping) implemented twice, once per path, with only one path ever audited. Key order had the same split and got caught; key escaping had it and did not. Fixed with detail::write_go_key, which routes keys through the same write_go_string as values. Gated both ways: new fixture 08_html_chars_in_keys.json makes section 14 7/7 across all three runtimes, and a test_json.cpp case goes red when the key path is reverted. Minor from the same review: compareUtf8's javadoc pointed at a worked example "at the Map serializer registration" that has never existed there — no U+FFFD or U+10000 appears anywhere in GoFormat.java. It now points at the test and the reference section that actually contain it. 347 Java tests, 248 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
BLOCKING, from the ninth review: the defect round 8 fixed existed a second time.
Round 8 routed the Variant writer's object keys through write_go_string. But
server.cpp has its own json_escape for the hand-written JSON paths — trace entry
names, /stats operator keys, error and warning text — and it lacked exactly the
same escapes: < > & and U+2028/U+2029. Reproduced on three live servers: an
operator named a<b&c>d came back raw in trace[].name and in /stats.operators
where Go emits a<b&c>d.
metrics_collector.cpp held a third copy, which additionally escaped no control
characters at all — it would have emitted raw bytes inside a JSON string.
So: one property, three implementations, and audits kept finding it in whichever
one had not been looked at yet. Adding the missing cases to copy two would only
have queued up copy three, so both now delegate to write_go_string and strip the
quotes it adds. One implementation remains. That is the actual fix; the escapes
were only the symptom.
Recorded as a discipline in the reference: when a string property gets a second
implementation in one runtime, merge rather than restate. Restating hands the
"keep these in sync" constraint to a future reader, and this task proved across
two consecutive rounds that nobody holds it.
Section 06's operator rename prefix now carries < & > so the hand-written path is
exercised by a gate rather than only by unit tests. Note the trace name still
cannot be pinned byte-exactly by section 14 — duration_ms is timing, so a trace
fixture can never be byte-stable there.
IMPORTANT, same review: httpDurationBucketFieldsAreOrderIndependentToday was a
tautology. It compared List.of("count","sum_ns") against itself sorted, reading
neither Go nor Java, so adding a third field to HttpStats.bucketView left all 347
tests green — it could not detect the drift its own message promised to catch. It
now reads the real HttpStats.snapshot() and goes red under that mutation.
Also: doc-gaps.md and index.md said 6 fixtures where there are 7, a stale hpp
line number from round 8's insertion, and two more restated counts. All four
replaced with "run the command" rather than a fresh number, matching the rule
this range already adopted for the others.
347 Java tests, 248 C++ cases, lint, codegen-check, cross-validate 55/55,
differential-fuzz 1000/1000.
…lizers
BLOCKING, from the tenth review, and it is my own consolidation's fault. Round 9
merged three escaping implementations onto write_go_string — correct in
principle, except write_go_string lacks Go's two-character \b and \f forms and
emits six-character hex instead. Keys had previously gone through RapidJSON's
Key(), whose escape table does handle both. So the merge fixed < > & on the key
path and simultaneously broke \b and \f there: one escaping divergence traded for
another, in the same commit that claimed to remove the class.
The reviewer proved the direction by reverting to Key() and observing b and f
become correct while < broke again. Both cases added.
The lesson goes in the reference: before consolidating onto an implementation,
verify that one is correct for the WHOLE rule table, not just for the characters
the last round happened to find. The two fixtures added in rounds 8 and 9 were
drawn from previously-found character classes; the new
09_control_chars_in_keys.json is drawn from Go's escape table instead — the five
two-character forms, hex forms whose digits exceed 9, the lowest and highest
control characters, the separators, quote and backslash, in keys and values both.
It goes red against either regression.
IMPORTANT, same review: Java emitted uppercase hex where Go emits lowercase
("000B" versus "000b"), affecting 0x0B, 0x0E, 0x0F and 0x1A-0x1F in keys and
values. The CharacterEscapes override covered the HTML set and left control
characters to Jackson's default. C++ already agreed with Go, so this was
Java-only and invisible without a control-char fixture.
IMPORTANT: trace duration_ms used snprintf("%g"), which is six significant
digits, against Go's shortest-round-trip. Any operator slower than about a
millisecond diverged: 1234.567 ms printed as 1234.57, and 1000.001 as 1000. It
was the last numeric in that file not using the shared formatter, and section 06
could not see it because it strips duration_ms as timing.
Deleted two unreachable reimplementations of the same rules: json.cpp's
dump_impl (131 lines, whose key branch applied no escaping at all) and
server.cpp's jsonvalue_to_string (50 lines with its own number rule). Both were
dead — dump_json forwards to dump_json_fast and nothing called the other — but
they are what the next person writing a serializer would find first. "One
implementation" is now true of the tree, not just of the reachable set.
Three doc minors: a restated count inside the sentence forbidding restated
counts, and a section heading plus bullet still framing pine-cpp as naturally
satisfying the contract.
347 Java tests, 248 C++ cases, lint, codegen-check, cross-validate 55/55
(section 14 now 8/8 both pairs), differential-fuzz 1000/1000.
Eleventh review: 0 blocking, and the escaping class is closed. The reviewer
derived Go's full escape table from encoding/json's source rather than sampling,
built probes for 54 character classes, and checked each one as both an object key
and a string value across every JSON exit in all three runtimes — including the
hand-written paths, the CLI, and each endpoint. All byte-identical, and
additionally byte-identical to Go's own json.Marshal of the same map.
IMPORTANT, from that review: the duration_ms fix had no regression gate at all.
Reverting it to snprintf("%g") left sections 06, 09 and 14 green, all C++ tests
green, and the fuzz green, because every one of them strips duration_ms as
timing.
Correcting the reachability while fixing it, because the finding's examples
implied worse than reality: duration_ms is duration_us / 1000.0, so it carries at
most three decimals, and "%g" only truncates once the integer part reaches four
digits — operators slower than 1000 ms. A 400k-iteration bench operator measures
4.27 ms and prints identically both ways. So the fix is correct but no
cross-engine channel can be made to go red without a deliberately multi-second
operator, which is too slow for this suite. test_json.cpp now pins the formatter
directly at the magnitudes that matter, and the new section 06 check records that
it verifies shape only and does not discriminate — rather than implying teeth it
does not have.
Also from the review: the \b and \f cases added last round were covered only by
fixture 09, i.e. the slowest channel. A unit test now covers the five
two-character forms plus lowercase hex above 9, so the fastest channel catches
them; verified red when either case is removed. And one /stats key was
concatenated without json_escape — source literals only, no live bug, but it is
the line the next person copies.
Two doc fixes: doc-gaps.md contradicted itself on whether this range added
fixtures, and the reference now states the measured duration_ms reachability
instead of the sub-millisecond claim.
347 Java tests, 250 C++ cases, lint, codegen-check, cross-validate 55/55
(section 06 now 24/24 and 23/23), differential-fuzz 1000/1000.
Twelfth review: APPROVE, 0 blocking. The reviewer rebuilt Go's escape table from live json.Marshal output across the full 0x00-0x7f range plus the special code points, in both key and value position, and diffed it against pine-cpp and pine-java: zero differences on both. It also ran 176,400 compareUtf8 pairs against Arrays.compareUnsigned with no mismatch, and confirmed no duplicate output-byte rule implementation remains reachable or dormant in either runtime. The one important finding is an asymmetry I left behind. Last round I added a fast-channel C++ test for escaping with a comment saying fixture 09 was "the slowest channel" and that the assertion moves the property to the fastest one — and then did not do the same on the Java side. Deleting the control-character takeover from createGoCompatMapper leaves all 347 Java tests green while the output already diverges from Go, because Jackson emits uppercase hex for the nine code points whose hex contains a digit above 9. Only fixture 09 caught it, via cross-validate. Now covered symmetrically: the five two-character forms, lowercase hex for 0x0b and 0x1f, the HTML-safe set and the quote, asserted in keys and values alike. It goes red against both the deletion and an uppercase-hex change. Two minors: a two-space comment indentation slip, and a note on tests 6 and 7, which sort key lists before comparing. The reviewer verified both dimensions are fully covered by 14b's nested sequence comparison, so they are redundant rather than blind — recorded as such, including that they are not a substitute if 14b is ever removed. 348 Java tests, 250 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
…plication Thirteenth review: APPROVE, 0 blocking and 0 important — the first round with neither. It verified 8,451 code points against Go's own json.Marshal, in key and value position each, with zero mismatches, and reached two response positions no earlier round had probed: non-ASCII operator names sorting in /stats.operators (byte order holds above the BMP, and std::string's < is memcmp semantics even where char is signed), and the operator_detail subtree, which only appears with transform_by_lua and whose Java side is a Map.of — unordered, correct only because GoFormat.sorted wraps it. Two minors. The reference's channel list did not mention the Java escaping unit test added last round, which is the only check that catches uppercase hex on the Java side; both language's fast-path tests are now listed together, with the point stated plainly: escaping regressions should be caught by unit tests, with fixtures as backstop, because round 10's \b/\f accident was found the slow way. And write_go_key repeats write_json_value's buffer boilerplate. Worth a note rather than a merge: it is not a second copy of the escaping rules — those live only in write_go_string — and the two thread_local buffers must stay separate, since a key and its value are both live inside one write_json_value call. 348 Java tests, 250 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
…rrect output Fourteenth review: APPROVE, 0 blocking and 0 important for the second round running. Both minors are claims of mine that do not survive checking. The buffer note I added last round said the two thread_local StringBuffers must stay separate or the value would overwrite the key. That is wrong: RawValue copies into the output stream before returning, so they are never live at the same time. The reviewer merged them and got byte-identical output across 20,000 random nested documents on both the compact and pretty paths. The note now says the duplication is removable and gives the real (weaker) reason to keep it. Gate 14d was worse than useless and is gone. Its exponent check read repr() of the parsed float, and repr(1.23457e+06) is "1234570.0" — no "e" — so that branch could never fire. Its "exactly six significant digits" check would have failed on perfectly correct output above one second: 1000.01, 1000.02, and 448 more within the first five seconds. A check that cannot detect the defect but can fail on correct output is worse than no check at all. I tried a third framing before removing it — compare digit CAPACITY across engines, which is timing-independent and sound in principle — and it cannot discriminate either, because the available bench operator peaks near 4 ms, four significant digits, where %g and shortest-round-trip are identical. Reaching six digits needs a deliberately multi-second operator. So the honest outcome is recorded rather than papered over: all three attempts and why each failed are written where test 14d used to be, so nobody spends a fourth, and the property stays pinned by test_json.cpp's "trace duration magnitudes match Go" at the magnitudes no channel here can reach. Not every property can be gated cross-engine; saying so beats leaving a check that is permanently green or that cries wolf. 348 Java tests, 250 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
Contributor
🔍 PR 审查
未发现需要阻止合入的问题。JSON 键排序、map/struct 序列化分流、字符串转义及相关校验通道的改动整体一致。 验证情况
本次审查由 Codex 主链路 (gpt-5.6-sol) 完成。 |
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.
Closes #183.
The reported bug, and what it actually was
#183 reported that pine-java emits JSON object keys in insertion order while Go sorts them. True,
and fixed in the first commit. But the issue framed this as a Java key-ordering bug, and the
contract is actually response bytes on every path that emits JSON. That difference produced
everything else here.
Reproduced across three runtimes before touching anything:
Two rules decide the implementation, and getting either wrong yields a fix that looks right on
ASCII fixtures:
Maps sort, structs do not. Go's
encoding/jsonsorts map keys but leaves struct fields indeclaration order. pine-go's response envelope is a struct (
executeResponse) while the payloadsinside it are maps, so the envelope keeps declaration order and everything within it sorts. My
first attempt registered a serializer for every
Map, which movederrorahead ofitemsandbroke the partial-error byte-exact fixture.
/statscontains both rules in one tree, so it iswrapped branch by branch according to the Go type.
The sort key is UTF-8 bytes. Jackson's
ORDER_MAP_ENTRIES_BY_KEYSandTreeMapboth useString.compareTo, i.e. UTF-16 code units, which disagrees with UTF-8 above the BMP:so
compareToputs U+10000 first where Go puts U+FFFD first. Using the built-in feature wouldhave relocated the divergence from ASCII keys to emoji keys rather than removing it.
Also fixed, found by review rather than by me
/statsin both runtimes. Go builds it as a map, so its top level sorts, unlike/execute.pine-cpp hand-builds that JSON by concatenation and emitted source order at the top level, inside
server, and foroperators(which Go sorts by operator name).in pine-cpp. Two lacked the HTML-safe escapes for
<>&and the U+2028/U+2029 escapes; onealso escaped no control characters at all. They are now one function. Merging them is what closed
the class, after patching them one at a time did not.
six-character hex where Go emits the two-character backspace and form-feed escapes. Keys had
previously gone through RapidJSON's
Key(), whose table handles both, so routing keys through theshared function regressed a path that had been correct.
Go emits lowercase, affecting the nine control code points whose hex contains a digit above 9.
writeValueAsBytes. Jackson escapes above-BMP characters as surrogate pairs on the byte pathbut not on the String path, while Go and C++ both emit raw UTF-8.
trace[].duration_ms. Usedsnprintf("%g"), six significant digits, against Go'sshortest-round-trip.
Verification
At
995f97d8: 348 Java tests, 250 C++ cases (110,647 assertions), lint, codegen-check,cross-validate 55/55, differential-fuzz 1000/1000.
The strongest evidence is independent. A reviewer derived Go's full escape table from
encoding/json's source, then compared 8,451 code points, each as both an object key and astring value, against Go's own
json.Marshal, across every JSON exit in all three runtimes. Zeromismatches. A later round repeated it with 4,000 random key sets, also zero.
make fmt-checkwas not run: no clang-format locally, and it has no CI job either.The gates had to be fixed before the fix could be trusted
Nothing could catch #183 when it was filed. Three separate blind spots:
09-raw-byte.shwas titled "no normalization" and fell back to normalized comparison onmismatch, counting equality as a pass. That fallback existed to tolerate this very
divergence.
differential-fuzzcompared aftersort_keys=True, erasing the dimension entirely.sorted(...)flow contracts, so declaration order always alreadyequalled sorted order.
The third is the instructive one: after adding an ordering check, the fuzz still passed 1000/1000
against a deliberately broken serializer. A generator that only produces inputs already in the
expected shape cannot detect a bug about shape. The same mistake recurred four times in this
range: in the generator, in a fixture's operator names, in a
common_inputdeclaration, and in atest comparing a hardcoded list against itself sorted. Each was green against broken code.
The durable rule, now in
guides/ci-quality-baseline.md: a new check is not evidence until it hasbeen shown to go red against a mutant.
One check was removed rather than kept. A
duration_msformat gate could not fail on the defectbut could fail on correct output above one second. Three framings were tried; all three and why
each failed are recorded where the check used to be, so nobody attempts a fourth. That property is
pinned by a unit test at magnitudes no cross-engine channel here can reach.
Review history worth knowing before merge
14 independent blind review rounds in isolated fixed-commit snapshots, each with a fresh reviewer
inheriting no context. Five blocking defects were found. The last three were all in string
escaping, each in a copy of the rule the previous round had not audited, and one of them was
introduced by my own fix for the one before it. Rounds 13 and 14 both returned APPROVE with zero
blocking and zero important findings.
Six rounds found no code defect at all, only documentation claims invalidated by a later commit in
the same range. The reflection document was corrected three times for this; the response was to
stop restating derived numbers and point at the command instead.
Two terminal evidence audits ran. Both found no implementation defect. The second failed on my
own closure records, including a totals line that restated a derived number and got it wrong,
which is exactly the failure six rounds had just caught elsewhere, and a fix attributed to the
wrong commit. All seven items are repaired and recorded under
.code-review/, along with threegaps left standing rather than reconstructed: the three commits central to this task predate the
review-skill invocation and so have no contemporaneous staging freeze; a reviewer wrote scratch
files into the coordinator worktree despite being snapshot-scoped; and no blind report artifact
survives for any round, from three distinct causes, one of which was an ambiguous instruction of
mine.
Deliberately not fixed, each with a stated argument
layer, not output formatting. No file involved is in this range.
{"common":{},"items":[]}where Go and Java emit nulls.A value difference, not ordering or escaping.