Reject invalid storage_mode and non-string root config values (#187, #188) - #191
Merged
Merged
Conversation
, #188) ## #187: fail-fast, and the divergence was wider than the issue said The issue framed this as a storage_mode value problem. Measuring first showed the type half is not storage_mode-specific at all: pine-go rejects a non-string for EVERY root string field, because they are all declared `string` and encoding/json fails the whole unmarshal. The other two disagreed, each differently — field go java cpp storage_mode reject coerce/accept reject (throwing as_string) log_prefix reject coerce/accept ignore (is_string guard) _PINEAPPLE_VERSION reject coerce/accept ignore _PINEAPPLE_CREATE_TIME reject coerce/accept ignore so pine-cpp was inconsistent with itself, and pine-java invented values through asText() (123 became "123", a container became ""). All four fields now reject a present-but-wrong-typed value in all three runtimes. JSON null is accepted and leaves the default, matching Go, where decoding null into a string is a no-op. Writing the test for that found a fourth divergence nobody had noticed: _PINEAPPLE_CREATE_TIME did not exist in pine-java at all, so a config pine-go refused outright was silently accepted. Now parsed — metadata only, never read for behaviour, but present so the type rule is uniform. On top of that, storage_mode now accepts only "row", "column", or absent/empty; anything else is rejected with a byte-identical message in all three runtimes. Rejecting at config load rather than in the frame factory is deliberate: the dispatch rule stays exactly "only the exact literal column selects column storage" (#179), and validation makes the invalid value unreachable instead of complicating dispatch. Go's whitelist constants are duplicated in internal/config rather than imported from internal/dataframe, which imports config — that would be a cycle. ## #188: fixtures chosen by shape, not by what already broke All eight existing byte-exact fixtures existed because something had already failed (#180 added the number-format one, #183 the three escaping ones), so the global "byte-exact" claim rested on much narrower coverage. Four new fixtures cover shapes enumerated up front: validation-error envelope, deep nesting (maps in lists in maps), null values at every position, and a filtered 12-item projection. The error-envelope one immediately found a real pre-existing divergence: pine-cpp emitted {"common":{},"items":[]} where Go and Java emit nulls. Go returns `nil, &ValidationError{}` before any operator runs, so there is no projected result; pine-cpp set has_result unconditionally after execution. Fixed by clearing it on the validation path only — an ExecutionError mid-pipeline still carries the fields written up to the failure, which fixture 02 pins. Section 14's header now records what this channel covers and what it structurally CANNOT: anything with trace or /stats can never be byte-stable, because duration_ms and the counters are real measurements. That was attempted during #183 and abandoned; adding such a fixture would produce a check that fails on correct code. Section 21 is rewritten, because #187 inverts the contract it was written to assert. It previously pinned "an invalid value is accepted silently"; it now pins rejection parity, with the old expectation recorded rather than deleted since pre-#187 history still describes it. Its captures needed `|| rc=$?` guards: the section deliberately runs cases that must exit non-zero, and _env.sh sets -e, so an unguarded $(...) aborted the whole script at the first rejected case. Two error fixtures added for the rejection path (error parity now 32/32). 354 Java tests, 253 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, differential-fuzz 1000/1000. Every new gate verified red against its own mutation.
Reflection: memory/reflections/config-validation-and-byte-exact-coverage-187-188.md Two user-facing guides were stating the OPPOSITE of the new behaviour — both said an invalid storage_mode is silently accepted by all three runtimes and falls back to row. That was true before #187 and is now wrong, which makes it the most misleading kind of stale doc: a reader would conclude a typo is harmless. Rewritten to state that every other value is rejected, in the DSL at compile time and in all three runtimes at config load with a byte-identical message, with the old behaviour kept as a short labelled history. dag-engine.md's "six interpretation points" table described exactly the baseline this change removed, so it is replaced by three layers — type, value, dispatch — and a behaviour table covering every JSON input form. It now says explicitly that the type rule is not storage_mode-specific: pine-go rejects a wrong type for every root string field because encoding/json enforces it per struct field, and the other two were changed to match. Also records why the value whitelist sits in config validation rather than the frame factory (it keeps #179's dispatch rule unchanged and makes the invalid value unreachable), and why Go's whitelist constants are duplicated in internal/config rather than imported from internal/dataframe, which imports config. New reference/root-config-string-fields.md carries the type contract on its own, because it is not about storage_mode: the next person adding a root string field would not find it filed under one. It lists what to change in all four places, and records that _PINEAPPLE_CREATE_TIME did not exist in pine-java at all — a whole missing field is invisible when reading code, which is why the type matrix test found it and inspection had not. Four disciplines added. In ci-quality-baseline.md: enumerate fixtures by response shape rather than waiting for an incident (all eight byte-exact fixtures had been incident-driven, and the first shape-enumerated one found a real divergence instead of confirming parity), plus what that channel structurally cannot cover — anything with trace or from /stats can never be byte-stable; an exhaustive test matrix finds gaps that reading cannot; and under set -e every capture in a section that deliberately asserts failure needs `|| rc=$?`, or the script aborts on the first expected failure and prints no pass or fail at all. In investigation-to-fix-testing.md: a single runtime is not necessarily self-consistent — pine-cpp had three different treatments of four sibling fields, which is the second instance of the shape #183 recorded. The "measure the affected surface before starting" lesson is promoted to must/conventions.md rather than recorded a fifth time. Four consecutive tasks hit it, and this task's evidence is the sharpest: I wrote the issue myself and still scoped it too narrowly, because writing an issue asks which field shows the symptom while the affected surface is which fields the mechanism touches. doc-gaps closes both open entries and opens one: reviewer scratch copies from close-local-code-review have no owner for deletion, cost 1-1.7GB each, and hit the disk quota at 17GB accumulated across the last three audits.
Blind review found no code defect. Every finding is in what I wrote about the code, and the first one matters because it would mislead someone writing a test. I documented the wrong-TYPE rejection message as byte-identical across the three runtimes. It is not: pine-go never emits that text at all. Its rejection comes from encoding/json failing the whole unmarshal, surfacing as "JSON parse error: json: cannot unmarshal number into Go struct field ...", while only pine-java and pine-cpp emit "config field X must be a string". I had conflated the two layers — the VALUE-layer whitelist message genuinely is byte-identical three ways, and I carried that property across to the type layer without measuring it. Corrected in the reference and in the index entry that restated it. Both READMEs still described section 21 as asserting that invalid values are accepted silently, which is the contract this range inverted, and bench-cross-runtime.sh's comment still said all three fall back to row storage — accurate between #179 and #187, falsified by this range. That is the second time in three tasks that changing a cross-runtime behaviour left a correct comment elsewhere describing the old one. Three narrower corrections: my claim that all eight byte-exact fixtures were incident-driven is wrong (four came from the channel's founding commit b3be250; only the increments were incident-driven, which leaves the argument intact but the count wrong); dag-engine.md claimed three-way agreement on "every JSON input form"; and the reference excluded `debug` in a way that reads as "already aligned" rather than "still divergent". Most valuable finding of the round, and it is a divergence THIS RANGE INTRODUCED: duplicate JSON keys resolve last-wins in pine-go and pine-java but first-wins in pine-cpp's FlatMap, so {"log_prefix":"ok","log_prefix":123} is now rejected by two runtimes and accepted by the third. Before this range nothing inspected the type, so which duplicate won made no difference. Measured on all three. It sits outside the frozen scope — root string field TYPE handling — so it is recorded as an open doc-gap alongside `debug`, which still has the pre-#187 pattern, each with its own decision inputs rather than fixed quietly here. 354 Java tests, 253 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, differential-fuzz 1000/1000.
…vergence
Second review: 0 blocking again, and two findings are worth more than the rest.
The two new error fixtures asserted in prose that the rejection message is
byte-identical across the three runtimes while checking only message_contains. The
reviewer reworded pine-java's message and section 05 stayed green at 32/32 — the
claim had no gate at all. Section 05 has supported wrapping_exact all along and
another fixture already used it. Both fixtures now use it, and rewording Java's
message turns them red. This is the repository's own rule applied to itself: a
byte-exactness claim needs a channel that does no normalization, and prose is not
that channel.
A SECOND divergence introduced by this range: pine-go's encoding/json falls back to
case-INSENSITIVE struct-tag matching, so {"STORAGE_MODE":"colunm"} binds to
StorageMode and is rejected, while pine-java's root.has() and pine-cpp's
parent.find() match exactly and never see the key, so both accept. Identical
mechanism to the duplicate-key gap found last round: before this range nothing
inspected the type, so where Go bound the key had no consequence. Recorded with the
other two exceptions rather than fixed, since it is outside the frozen scope.
I had also mischaracterized `debug`. I wrote that Go rejects a wrong type while the
other two silently ignore it. Measured: pine-java's asBoolean() coerces, and
debug:1 or debug:"true" actually TURN DEBUG ON, while "yes" and [1] coerce to
false. Three behaviours, not two — which materially changes the effort estimate on
whether to align it.
Section 14's header still carried the "all eight fixtures were incident-driven"
over-claim that last round corrected in ci-quality-baseline.md, and that guide's
closing line points readers at this header before they add a fixture. Fixed in both
places now.
Three minors: both user guides asserted the byte-identical message without the
value-versus-type layer qualifier that the reference already draws; Go formats the
rejected value with %q where the other two concatenate raw, so the value-layer
message differs for values containing a quote or backslash (unreachable by any
all-ASCII fixture, same class as #183's escape-vs-raw); and the Apple DSL's
compile-time gate covers storage_mode only, so "the DSL cannot emit a config the
runtimes reject" is false for the other three root string fields.
354 Java tests, 253 C++ cases, Go suite, lint, codegen-check, cross-validate 57
PASS across 21 sections, differential-fuzz 1000/1000.
Third review: 0 blocking. Three important findings, and the first led me into a mistake worth recording. A THIRD divergence this range introduced: with two root fields wrong-typed at once, each runtime named a different field, because each iterated in its own order. Before this range pine-cpp threw only for storage_mode and pine-java coerced everything, so no ordering was observable — the range created the dependency. This repo already treats first-error priority under simultaneous violations as an external contract. My first fix was wrong, and measuring caught it. I reordered both runtimes to pine-go's RootConfig struct declaration order and wrote comments asserting that was the rule. Then the same three bad fields in two different JSON key orders made pine-go name log_prefix once and storage_mode the other time: encoding/json reports whichever wrong-typed field appears first IN THE DOCUMENT, so no fixed check order can reproduce it. The reviewer's diagnosis was right; its suggested remedy, and my implementation of it, rested on a wrong model of Go. What survives is narrower and true: pine-java and pine-cpp share one order and are each pinned to it by a unit test, verified red by physically moving the C++ check block. pine-go is documented as input-order-dependent and explicitly outside the contract. I also deleted the error fixture I had written for it — section 05 enforces message_contains on all three engines and pine-go cannot match, which is precisely why this property belongs in unit tests. That deletion is the useful part: a fixture that can never pass is worse than no fixture. Nothing gated the type-layer message at all. The two value-layer fixtures use wrapping_exact, but grep found no fixture containing "must be a string", so pine-java could have been reworded with every gate green. A new fixture pins it for java and cpp, with Go excluded by wrapping_exact_engines because its encoding/json phrasing differs by construction rather than by divergence. My %q caveat was too broad: it exempted non-ASCII, but only PRINTABLE non-ASCII is unaffected. U+00A0, U+00AD, U+200B, U+2028 and U+2029 all diverge, and those are the more reachable class precisely because they are invisible in an editor — a value pasted from a web page carries them. Three minors: bench-cross-runtime.sh's correction had been prepended without deleting the false present-tense sentence, leaving a splice that both asserted and denied the same thing; doc-gaps' heading said two exceptions where its body said three, with the new bullet indented under the wrong argument; and index.md never received the entry at all, on the route startup.md points readers down. 355 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, differential-fuzz 1000/1000.
Fourth review: 0 blocking, and it found the fourth instance of the class the previous three kept surfacing — this range added validation whose outcome depends on parser details that were previously inert. pine-cpp kept the storage_mode whitelist in validate_config, which runs at the END of load_config_from_json, after require_obj, parse_operator, parse_metadata and apply_registry_traits have each had a chance to throw. pine-go and pine-java both check it first in validate(). So a config violating storage_mode AND something else reported different errors: pine-cpp named the operator's missing type_name where the other two named storage_mode. Which error a config reports first is externally observable, and this repo already treats first-error priority under simultaneous violations as a contract. Unlike the duplicate-key, key-case and Go-document-order divergences — which are properties of three different JSON parsers — this one is entirely ours, so it is FIXED rather than recorded: the whitelist moved to parse time, immediately after storage_mode is read. A new error fixture pins it with wrapping_exact across all three, verified red by deferring the check again. The ordering tests I added last round only pinned POSITION 1. The reviewer swapped storage_mode with log_prefix in pine-cpp and every gate stayed green — 254 C++ cases and section 05 — while the two runtimes blamed different fields. Three of four positions were free under a test whose own source comment claimed the order was pinned. Both tests now assert every adjacent pair, which locks all four positions with three assertions, and the swap that used to be invisible now fails. Three minors: both user guides asserted the value-layer message is byte-identical with no qualifier, while the reference already records that %q makes it differ for values containing a quote, backslash, control character or non-printable Unicode; doc-gaps' new entry had broken list nesting that rendered two of the four divergences under the wrong parent, with 待决策 covering only two of them and the scope line still saying two; and the reflection said seven interpretation points where the whitelist is three rather than one — nine by the same accounting dag-engine.md uses — and still carried the "all eight fixtures were incident-driven" claim corrected elsewhere in round 1. 355 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, error parity 34/34, differential-fuzz 1000/1000.
Fifth review, and the first blocking finding of this range — introduced by the
previous round's fix for the fourth.
Moving validate_storage_mode to parse time made it outrank operator errors, which
was the point. But it landed BETWEEN the storage_mode and log_prefix reads, so a
config with both an invalid storage_mode value and a wrong-typed log_prefix
reported the value error in pine-cpp while pine-go and pine-java reported
log_prefix's type error. That is java != cpp, the one pairing the new ordering
tests and the source comment both claim is aligned, and it holds regardless of key
order, so it is a fixed disagreement rather than another parser property.
Nothing caught it, and the reason is worth keeping: the adjacent-pair ordering
tests use two wrong-TYPED fields, and the precedence fixture pairs storage_mode
with an OPERATOR error. Neither mixes the type and value layers, which is exactly
where the defect lived. A new fixture does, pinned with wrapping_exact for java and
cpp, verified red by moving the call back.
The call now sits after all four type checks and before parsing, and the comment
states both constraints it has to satisfy at once, because satisfying one is what
broke the other:
1. after every root-field type check — otherwise the value error outranks a type
error that Go and Java report first;
2. before operator parsing — otherwise a co-occurring operator error outranks the
value error, which is what round 4 fixed.
Two minors: dag-engine.md and pine-cpp-runtime.md still cited the whitelist as
living in validate_config, the one function the new comment says it must not be in;
and the %q caveat still justified itself by saying two user guides assert
byte-equality without qualification, after the same range qualified both.
355 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57
PASS across 21 sections, error parity 35/35, differential-fuzz 1000/1000.
Sixth review, sixth instance of the same class, and this one is not being fixed. With an invalid storage_mode AND a nested field of the wrong type — for example $metadata.common_input given as a string — pine-go and pine-java report the nested type error while pine-cpp reports the value error. Measured across eight nested fields. The cause is that encoding/json fails on a type error at any depth, while validate_storage_mode sits after only the four ROOT type checks. The reviewer had already tested the move that looks correct: putting the call before apply_registry_traits fixes all eight nested cases and re-breaks what round 4 fixed, so a co-occurring operator error wins again. The two constraints cannot both be met by relocating one call, because parse_operator interleaves type-shaped and semantic throws throughout — satisfying both needs pine-cpp's config loading split into a type pass and a value pass. I verified that reading independently and put the scope choice to the user, who chose to record it rather than restructure config loading inside this range. So the code is unchanged and three places now state the limit precisely: doc-gaps carries it as a fifth entry including the remedy that fails and why, dag-engine's "type layer before value layer" claim is qualified to root level, and the source comment gains a KNOWN LIMIT block so the next reader does not attempt the move. Also recorded: the three precedence gates added in rounds 3-5 all use root-level fields, so none of them can fail on this. The reviewer confirmed each does fail on the mutation it targets, so they pin what they claim — the claim was just narrower than the invariant the comment asserted. That gap between "what the test pins" and "what the comment says" is the shape that produced five of the six findings in this range. 355 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, error parity 35/35, differential-fuzz 1000/1000.
Seventh review: 0 blocking, 0 important. The reviewer verified by mutation the claim round 6's decision rests on — that moving validate_storage_mode before apply_registry_traits fixes the nested cases and re-breaks operator precedence — and confirmed both constraints are real, each with exactly one gate, in the right layer. It measured over thirty error-precedence combinations and found the only nested-versus-value divergences are the accepted class. All three minors are counts that went stale when round 6 inserted a fifth doc-gaps entry: index.md still said four, the reflection's Promotion and Follow-up sections still said seven interpretation points after the body was corrected to nine, and doc-gaps' "the last three are newly introduced" stopped pointing at the intended three once the nested entry landed between them — so the sentence contradicted the list immediately below it. The second is the same every-copy-of-a-claim discipline #179 recorded, recurring inside the document that records it. Also fixed something the reviewer explicitly declined to report: my note said "three unit tests" assert byte-parity without qualification, and pine-cpp's test never mentions byte-identity. It judged the over-count as erring toward less risk and therefore not worth reporting; I fixed it anyway, because it is a number I wrote and it is wrong. Left alone, also from the declined pile: bench-cross-runtime.sh's parenthetical about when the --modes check was added. The reviewer traced it to e3627ec, which predates #179, making the sentence ambiguous rather than false — and the same ambiguity predates this range. 355 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, error parity 35/35, differential-fuzz 1000/1000.
…radicted itself Eighth review: 0 blocking, 0 important for the second round running. Both minors are in text I wrote to describe the accepted nested-ordering limitation, and both contradict themselves rather than merely being stale. dag-engine.md said "the other two" place their value check before nested parsing, in the same sentence that concludes only pine-cpp reports the value error. Those cannot both be true. Measured: Config.load calls parseRoot — which parses every nested structure — before validate(), so pine-java's whitelist is strictly after nested parsing and it reports the nested type error like Go. Only pine-cpp checks early. doc-gaps.md had this right and named pine-cpp alone; the dag-engine sentence generalized it wrongly. The reflection still read "(three dispatch + three parse + ONE whitelist)" on the very line whose count the previous commit corrected to nine, so the number and its arithmetic disagreed inside one sentence. That second one is the every-copy-of-a-claim discipline failing for the fourth time in this range, now one line below the line that fixed it. The reason is worth recording: each round I searched for the NUMBER, never for the ARITHMETIC that produces it, so a parenthetical restating the same claim in different notation survived every pass. Searching for "七个" cannot find "三 + 三 + 一". 355 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, error parity 35/35, differential-fuzz 1000/1000.
Found while sweeping my own restatements before the ninth review, using the lesson from the eighth: search for the CLAIM, not the number. root-config-string-fields.md asserted the value-layer message is byte-identical three ways in one paragraph and qualified that same claim twenty lines later, so a reader stopping at the first statement would write a test the caveat says cannot hold. Now cross-referenced.
Ninth review: 0 blocking. The important finding is that I claimed a gate stronger than the mechanism provides, for the third time in this range. Five fixture descriptions argue that wrapping_exact closes the byte-identical gap message_contains left open. But section 05 implements wrapping_exact with grep -qF — containment, not equality. The reviewer appended " (see docs)" to pine-go's message, producing a genuine byte divergence, and section 05 stayed 35/35 green. Rewording mid-message IS caught, so the gate has real value; it pins the message prefix. All five descriptions now say that, with the measurement that shows it. That is the same shape as two earlier findings here: the ordering tests pinned only position 1 while their comment claimed the whole order, and the precedence fixtures used root fields only while the comment claimed the invariant generally. Each time I asserted a property and reached for a mechanism that tests something weaker. dag-engine.md's sentence about pine-java being unaffected by the nested ordering limit holds for leaf fields only. For CONTAINER-typed fields pine-java joins pine-cpp, because parseRoot cannot detect those type errors at all: .fields() on a TextNode yields an empty iterator and readStringList silently returns nothing, neither throwing, so the whitelist fires first. Measured — pipeline_group as a string gives go=type, java=value, cpp=value. Those container cases already diverged before #187, so this is a wrong attribution rather than a regression, but it is the second consecutive round where the sentence fixing an overgeneralization overgeneralized in a new direction. Also repaired a sentence in the reference doc where two back-to-back parentheticals had split a verb from its object across a line break. Finally, completed the one measurement the reviewer could not: the external cleaner deleted its snapshot mid-mutation, leaving open whether fixture 10 gates the has_result fix. Reverting that fix turns section 14 red on 10_validation_error_envelope for Go vs C++ while Go vs Java stays 12/12. It does. 355 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, error parity 35/35, differential-fuzz 1000/1000.
Tenth review: 0 blocking, and the first round in which every gate-strength claim verified without correction — the reviewer probed each with the weakest mutation rather than the obvious one (swapping only the two _PINEAPPLE_* reads, which no fixture touches; moving the value check before _PINEAPPLE_CREATE_TIME alone) and each intended gate still went red. The one important finding is that round 9's narrowing overcorrected. I had written that pine-java joins pine-cpp on the nested ordering limit only for container-typed fields. Measured, eight scalar LEAF fields do too. The real boundary is whether the read throws: parseOperatorConfig takes type_name, recall, debug, consumes_row_set, mutates_row_set, additive_writes_row_set, for_branch_control and skip through asText() or asBoolean(), which coerce silently, so the whitelist fires first; readStringList throws, so array fields match pine-go. Confirmed both directions — type_name=123 gives java the value error, sources="x" gives it the type error. dag-engine.md now carries a three-row table keyed on the read mechanism instead of a claim about field shape, and doc-gaps and the source comment say the same thing. That is the third consecutive round correcting a sentence that was itself written to correct an overgeneralization, so the table is deliberately mechanical: it names the accessor per row, which is checkable, rather than a category, which is not. Two aborted attempts preceded this round and neither was a review outcome. The external /tmp cleaner deleted one snapshot between the reviewer's directory listing and its first file read; it reported zero coverage rather than a clean verdict, which is why this run is provisioned outside /tmp. The first retry then died on an API 401 before doing any work. 355 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, error parity 35/35, differential-fuzz 1000/1000.
Four consecutive review rounds failed to state one boundary correctly in prose — when pine-java agrees with pine-go about a nested type error outranking the storage_mode value error. Each attempt described it as a CATEGORY and each was wrong: "all three agree", then "leaf fields agree, containers do not", then "only containers do not". The boundary is neither, because it is the ACCESSOR: asText() and asBoolean() coerce and never throw, so the whitelist fires first; readStringList throws, so array fields match Go. Rather than write a fifth sentence, the claim is now a test that asserts it over all seven coerced operator fields and one throwing one, verified red by making type_name throw. A sentence that becomes false stays quiet; this fails. The discipline is recorded in investigation-to-fix-testing.md with the four attempts tabulated: when the same cross-runtime description is corrected by review twice, stop rewording it and write the assertion. dag-engine.md's table now points at the test rather than standing alone, and names the accessor per row — checkable — instead of a field category, which is not. Note the test asserts an ACCEPTED limitation rather than demanding a fix; the residual pine-cpp divergence is tracked in doc-gaps.md and needs a two-pass config parse, which the owner decided against for this range. 356 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, error parity 35/35. differential-fuzz hit one genuine divergence during this round and it is NOT in this range: go emits 4611686018427388000 where java emits 4611686018427387904 for the same float64 (2^62), produced by transform_by_lua under a valid storage_mode, and reproducing at the range base. Filed as issue #190 with the case preserved under .code-review/preserved/, since the /tmp copy will be reaped. Subsequent runs were 1000/1000 four times — the failing seed was not printed in the summary, which #190 also asks to fix.
…laims Eleventh review: 0 blocking. The new test, the mechanism table and the discipline note all verified — the test is a genuine gate in both directions, the table is correct row by row against measurement, and the four-attempt history matches the commits. The findings are all in what surrounds them. Two important. My round-10b insertion landed inside the three-layer table, between the value-layer and dispatch-layer rows, so the dispatch row ended up orphaned several paragraphs below and the table the prose exists to explain was broken by it. And that prose said .fields() throws IllegalArgumentException while row three of my own table, two lines further down, says .fields() yields an empty iterator and does not throw — the sentence contradicted its own table. Four minor. Two hardcoded counts in the reflection went stale across four later commits (3 and 2, against 5 Java tests, 3 C++ cases and 5 error fixtures), and they sit on the lines either side of the one stating the rule against hardcoding counts. Two more restatements still claimed all eight byte-exact fixtures were incident-driven, after round 1 corrected that in two other places. Fixing these took four attempts and every intermediate step broke something else: a claim spanning two lines half-corrected, two edits colliding into a sentence with no object, and a replacement that swallowed the clause introducing the section-14 boundary. Each was caught by re-reading the rendered paragraph and checking marker parity — the discipline this range recorded after round 5, which I had to actually run here rather than cite. Worth noting what the reviewer did: it rejected two claims raised by its own delegated verification pass, re-deriving each against the code and git history before deciding. That is the opposite of the failure this range kept repeating. 356 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, differential-fuzz 1000/1000.
Round 11's two important findings were both created by round 10b's edit to this same 19-line block: it was inserted inside the three-layer table, orphaning a row, and it asserted .fields() throws while its own table two lines below said the opposite. That is three consecutive rounds of findings in one passage. The mechanism now exists in four places — a mutation-verified test with twelve assertions, doc-gaps.md, investigation-to-fix-testing.md, and this hand-written table. Only the test goes red when it is wrong. So the prose copy is deleted and replaced with a pointer to the test, which is what the previous commit already established as the durable form. The discipline generalizes one step further than the note added last commit: not just "write the assertion instead of rewording" but "then delete the prose copy" — keeping both means keeping a copy that can silently drift, and this passage drifted three times in three rounds. Recorded in place. doc-gaps' cross-reference now points at the test rather than the deleted table. 356 Java tests, 254 C++ cases, lint, codegen-check clean.
Twelfth review: 0 blocking. Two important findings, both consequences of the previous commit's deletion. The value-layer table cell now ended mid-sentence: the clause introducing the mechanism stopped exactly where the deleted paragraph used to continue it. Closed at the last complete sentence, with the pointer carrying the rest. More substantively, the pointer claimed the test asserts the rule per-field and that changing any reader turns it red. Both were overstated — `skip` was missing from the coerced-field list and the `.fields()` container reader had no assertion at all. The reviewer proved it by making each throw and getting a fully green suite. Both are now covered, and the pointer says "three accessor classes" rather than "per-field", because that is what the test actually does. Writing the container assertion took three attempts and the third failure is worth recording. Using `pipeline_group` was wrong because a wrong-typed one also makes validate() throw "pipeline_group is empty", indistinguishable from the whitelist message, so it passed for the wrong reason. Switching to `flow_contract` still stayed green under mutation, because I PREPENDED the key while the base config already contains it — and Jackson's readTree is last-wins, so the valid object silently overrode my string and the container path was never exercised. That is the duplicate-key resolution this range documents as an accepted limitation, quietly defeating a test about the same family of mechanism. The fix replaces the existing key instead of prepending, and both traps are written into the test so the next person does not rediscover them. The minor finding, a 161-char comment against a 110-column limit, is left: no CI job checks line length and the missing clang-format job is already a tracked doc-gap. 356 Java tests, 254 C++ cases, Go suite, lint, codegen-check, cross-validate 57 PASS across 21 sections, differential-fuzz 1000/1000.
Terminal audit returned FAIL, and this is the one finding in it that touched code. Round 12 reported a 161-character comment line in config.cpp against the 110-column ColumnLimit in .clang-format. I recorded the other findings from that round and silently let this one go, so it was still live at HEAD — never fixed, never disproved, never written down as accepted. It fell out at exactly the point the auditor predicted the evidence chain was weakest: where the only record of a finding is my own transcription of a report that no artifact preserves. That prediction landing is worth more than the fix. Nothing gates it, which is why nothing else caught it: clang-format is not installed locally and there is no CI job for it, already a tracked doc-gap. The audit's other three findings were record defects, all corrected in .code-review/closure-ledger-187-188.md and the scope addendum: a round/commit attribution that named a commit which never touched the file, two self-initiated commits credited as round fixes while my own evidence-gap record classified them otherwise, and a "rounds 7 through 12 found no code defect" claim that this ledger's own r12 row falsifies — sitting in the paragraph that justified stopping the loop. 254 C++ cases, lint clean.
Contributor
🔍 PR 审查
已检查各运行时的 本次审查由 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 #187. Closes #188.
#187 — reject invalid config values instead of silently accepting them
Before this, a typo in
storage_modeproduced a working engine whose memory and performance profilewas the opposite of what you asked for. #179 had aligned the direction of the silent fallback; the
silence itself remained.
Measuring first changed the shape of the fix. The type-handling divergence was not
storage_mode-specific — it affected every root string field, and the three runtimes disagreed threedifferent ways:
storage_modeas_string())log_prefixis_string()guard)_PINEAPPLE_VERSION_PINEAPPLE_CREATE_TIMESo pine-cpp was inconsistent with itself, and pine-java invented values through
asText()—123became
"123", a container became"". All four fields now reject a wrong-typed value in all threeruntimes;
nulland an absent key both yield the default, matching Go.Writing the test for that surfaced a fourth divergence nobody had noticed:
_PINEAPPLE_CREATE_TIMEdid not exist in pine-java at all, so a config pine-go refused outright was silently accepted. A
4-field × 6-type matrix found it; reading code had not.
The value whitelist is deliberately in config validation rather than the frame factory, so #179's
dispatch rule ("only the exact literal
columnselects column storage") is untouched — validationmakes the invalid value unreachable instead of complicating dispatch.
#188 — fixtures chosen by shape, not by what already broke
All eight existing byte-exact fixtures existed because something had already failed. Four new ones
cover shapes enumerated up front, and the first one immediately found a real pre-existing
divergence: pine-cpp emitted
{"common":{},"items":[]}on a validation error where Go and Java emitnull, because it sethas_resultunconditionally after execution while Go returnsnil, &ValidationError{}before any operator runs.Section 14's header now records what that channel covers and what it structurally cannot: anything
with
traceor from/statscan never be byte-stable, becauseduration_msand the counters are realmeasurements. That was attempted during #183 and abandoned — adding such a fixture yields a check that
fails on correct code.
The expensive discovery, and why this took twelve review rounds
Adding a validation check makes previously-inert parsing differences observable. Nothing inspected
config value types before, so it did not matter that the three runtimes resolve duplicate keys
differently, match key case differently, or reach fields in different orders. The check turned each of
those into a visible divergence. Six instances were found:
Fixed — pine-cpp validated too late (a co-occurring operator error won), then the fix for that
validated too early (a wrong-typed field's error lost to it). Both now gated by error fixtures,
verified red by reverting.
Accepted, recorded in
doc-gaps.mdwith decision inputs — duplicate-key resolution (last-wins inGo/Java, first-wins in pine-cpp's
FlatMap), Go's case-insensitive struct-tag fallback, which field Gonames when several are wrong-typed (it follows JSON document order, so no fixed check order can match
it),
debug's three-way split (pine-java'sasBoolean()genuinely turns debug on for1), andnested type-error-versus-value-error precedence, which needs a two-pass config parse — your call was to
record rather than restructure config loading inside this range.
The generalizable lesson, now recorded: the affected surface of adding a check includes everything
whose relative order that check newly exposes. Asking which fields are read does not reveal it.
Verification
356 Java tests, 254 C++ cases, the Go suite, lint, codegen-check, cross-validate 57 PASS across 21
sections, error parity 35/35, byte-exact 12/12 both pairs, differential-fuzz 1000/1000. Every new gate
verified red against its own mutation.
make fmt-checknot run — no clang-format locally and no CI jobfor it, an existing tracked gap.
Review history worth knowing before merge
Twelve independent blind review rounds in isolated fixed-commit snapshots, plus two aborted attempts
(an external
/tmpcleaner deleted one snapshot before the reviewer read a single file — it reportedzero coverage rather than a clean verdict, and snapshots moved off
/tmpafter that).Rounds 7-11 found no code defect. Every finding was a sentence I wrote describing one accepted
limitation, and each correction introduced the next: a stale count, a wrong per-runtime attribution, an
arithmetic that contradicted the number on its own line, an overcorrection in the opposite direction,
then an insertion that structurally broke the table it was explaining. Four attempts to state one
boundary in prose, four failures — all because I kept describing a mechanism as a category.
The fix was to stop writing sentences about it. The rule now lives in
StorageModeValidationTest.nestedTypeErrorPrecedenceDependsOnWhetherTheReadThrows, which asserts allthree accessor classes (
asText()/asBoolean()coerce,readStringListthrows,.fields()yields anempty iterator), and the prose copy was deleted rather than maintained in parallel. Round 12 then
caught that the test's own comment overstated its coverage —
skipand the container reader wereunasserted — which is closed and mutation-verified.
Three separate gate-strength claims of mine turned out weaker than advertised: ordering tests pinning
only position 1, precedence fixtures using root-level fields only, and
wrapping_exactpinning aprefix rather than byte-identity (
05-error-parity.shusesgrep -qF, so a suffix-only divergencepasses). All three are now described as what they actually test.
Three terminal evidence audits, all three FAIL, none on the implementation. Every failure was a
derived restatement in my closure records — a misattributed commit, self-initiated commits credited as
review fixes, a false "no code defect" claim in the paragraph justifying stopping, and once a claim to
have fixed something where my
.replace()had silently matched nothing. One real finding did fall outof the chain at the point the auditor predicted was weakest, and is fixed. Derived counts, ordinals and
the commit list are now removed from those records in favour of the git commands that derive them.
Found in passing, filed separately
differential-fuzzsurfaced a genuine Go-vs-Java number-spelling divergence at 2^62 fromtransform_by_luaunder a validstorage_mode— Go emits4611686018427388000, Java the exact4611686018427387904. Outside this range, reproduces at the base, filed as #190 with the casepreserved because the
/tmpcopy will be reaped. It also asks that the fuzz runner print the failingseed, which it currently does not.