From adc88186af99a165d5ed06c12da87c2484b00a8f Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 3 Aug 2026 07:10:00 +0300 Subject: [PATCH] fix(internal/harness): exclude permutations that change meaning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two-order oracle compiles a source as written and again with every mapping reversed, and reports what the permutation changed. That rests on the rewrite being meaning-preserving. reverseMappings documented one case where it is not — a duplicate mapping key, whose meaning depends on the order being changed — and excluded it. Three more went unexcluded, and in each the two compiles differ because of the rewrite rather than because of a lowering, so the oracle blamed the compiler. yaml.Marshal does not preserve a flow-style implicit null: "{A}" comes back with an empty string for its value, so the second compile read a string where the first read null. Both arms are now compiled from the encoder's output, leaving declaration order as the only difference. Reversing a mapping can carry an alias above the anchor it names, which YAML forbids, so the permuted source no longer parsed at all and the parse failure was reported as order dependence. The rewrite now re-parses what it produced and declines a source whose permutation does not survive it, which covers any later ordering rule of the same kind without enumerating it. diagnosticSet identified a finding partly by Provenance.Pointer, which is documented to hold either a structural pointer or line:col, and a permutation moves a construct to a different line by design. A positional pointer is now replaced rather than dropped, so a permutation that changes how many findings were reported still shows. All three were latent: the committed corpus reaches none of them and the sweep was green. Each is pinned by a case that fails without its own fix and by nothing else. The reachability guard now covers both arms, since the baseline can go quiet on its own. TestReverseMappings_AliasIsNotFollowed asserted the anchor-above-alias output as correct; its fixture moves to a sequence, whose order the rewrite leaves alone, so it still pins that an anchored mapping reverses once. Closes #241 --- internal/harness/harness.go | 2 +- internal/harness/order.go | 114 ++++++++++++++++--- internal/harness/order_test.go | 197 ++++++++++++++++++++++++++++++--- 3 files changed, 282 insertions(+), 31 deletions(-) diff --git a/internal/harness/harness.go b/internal/harness/harness.go index fb0208c..b10aeef 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -81,7 +81,7 @@ func Check(ctx context.Context, spec string, data []byte) (res Result) { // Last, and after deterministic: this one permutes the input, so a compiler // that is not even same-input-same-output should be reported as that rather // than as order-dependence. - if detail, ok := orderInvariant(ctx, spec, data, doc); !ok { + if detail, ok := orderInvariant(ctx, spec, data); !ok { return Result{Spec: spec, Outcome: OutcomeOrderDependent, Detail: detail} } return res diff --git a/internal/harness/order.go b/internal/harness/order.go index 4fe2873..f999306 100644 --- a/internal/harness/order.go +++ b/internal/harness/order.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "sort" + "strings" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -19,8 +20,15 @@ import ( // depth. const maxReverseDepth = 512 -// orderInvariant compiles data as written and again with every mapping's entry -// order reversed, and reports what the permutation changed beyond source order. +// positionPlaceholder stands in for a provenance pointer that locates a +// construct by source position. The leading control byte is what makes it a +// placeholder rather than a value: no producer spells a pointer with one, so it +// cannot collide with a pointer that means itself. +const positionPlaceholder = "\x01position" + +// orderInvariant compiles a source twice — once with its mappings as declared, +// once with every mapping's entry order reversed — and reports what the +// permutation changed beyond source order. // // It is the general form of the two-order diff CLAUDE.md prescribes. // `deterministic`, next to it, looks like this and is a different property: it @@ -29,26 +37,44 @@ const maxReverseDepth = 512 // pointer first — the shape of the pointer collisions in #108 and #112, which // produce no diagnostic in either order and leave pass/validate clean on both. // -// Two limits belong here so the oracle is not over-trusted: +// Both arms are compiled from the encoder's output rather than one from the +// source and one from the rewrite. The encoder does not preserve every spelling +// — a flow-style implicit null comes back carrying an empty string — and +// comparing against the source would read that rewriting as a lowering that +// depends on order. Passing both sides through it leaves declaration order as +// the only difference between them, which is the question being asked. // -// Reversing a mapping is meaning-preserving only while its keys are distinct. A -// document with duplicate mapping keys resolves to the last one (#95), so -// reversing changes which declaration wins and the two compiles legitimately -// differ; such sources are excluded rather than reported. Sequences are left -// alone throughout — allOf precedence, oneOf variant order and prefixItems -// positions are all semantic, and reversing them would change the document's -// meaning rather than only its spelling. +// The limits belong here so the oracle is not over-trusted. Reversing a mapping +// is meaning-preserving only while its keys are distinct: duplicate keys resolve +// to the last declaration (#95), so reversing changes which one wins and the two +// compiles legitimately differ. Such a source is excluded rather than reported, +// as is one whose permutation no longer parses — see reverseMappings — and one +// whose re-encoding will not compile, which leaves nothing faithful to compare +// against. Sequences are left alone throughout: allOf precedence, oneOf variant +// order and prefixItems positions are all semantic, and reversing them would +// change the document's meaning rather than only its spelling. // // And it proves order-independence only for the constructs its input contains, // which is why it runs over the corpus rather than over one hand-written spec. -func orderInvariant(ctx context.Context, spec string, data []byte, doc *ir.Document) (string, bool) { +func orderInvariant(ctx context.Context, spec string, data []byte) (string, bool) { + baseline, ok := reencodeMappings(data) + if !ok { + return "", true // the source does not survive a parse and re-encode + } reversed, ok := reverseMappings(data) if !ok { - return "", true // not a document whose permutation is meaning-preserving + return "", true // its permutation would not be meaning-preserving } - if bytes.Equal(reversed, data) { + if bytes.Equal(reversed, baseline) { return "", true // nothing to permute; the oracle has no question to ask } + doc, _, err := compile(ctx, spec, baseline) + if err != nil { + return "", true // the re-encoding does not compile, so there is no baseline + } + if doc == nil { + return "", true // nor does a source the compiler declines outright + } other, otherDiags, err := compile(ctx, spec, reversed) if err != nil { return "recompile permuted: " + err.Error(), false @@ -109,16 +135,46 @@ func diffOrderInvariants(first, second *ir.Document, secondDiags []ir.Diagnostic // reverse — which is invariant #7's source ordering reaching the message rather // than a lowering that depends on order. Severity, code and pointer identify the // finding without that. +// +// A pointer spelled line:col is excluded for the same reason. Provenance.Pointer +// admits either a structural pointer or a source position, and a permutation +// moves a construct to a different line by design. Replacing it rather than +// dropping the field keeps the finding in the multiset, so a permutation that +// changes how many were reported still shows. func diagnosticSet(diags []ir.Diagnostic) []string { out := make([]string, 0, len(diags)) for _, d := range diags { + pointer := d.Provenance.Pointer + if isSourcePosition(pointer) { + pointer = positionPlaceholder + } out = append(out, fmt.Sprintf("%s\x00%s\x00%s\x00%d", - d.Severity, d.Code, d.Provenance.Pointer, d.Provenance.Source)) + d.Severity, d.Code, pointer, d.Provenance.Source)) } sort.Strings(out) return out } +// isSourcePosition reports whether a provenance pointer is a line:col position +// rather than a structural pointer. +func isSourcePosition(pointer string) bool { + line, col, ok := strings.Cut(pointer, ":") + return ok && isDigits(line) && isDigits(col) +} + +// isDigits reports whether s is a non-empty run of ASCII digits. +func isDigits(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + // sourceOrderedCollections orders the collections a mapping's entry order // decides, so comparing two permutations of one document does not report their // own permutation. @@ -137,10 +193,32 @@ func renderExample(e ir.Example) string { return fmt.Sprintf("%s\x00%s\x00%v", e.Name, e.ExternalURL, e.Value) } +// reencodeMappings returns src parsed and re-encoded with its entry order +// intact: the same normalization reverseMappings applies, minus the permutation. +// It is what the permuted source is compared against, so a spelling the encoder +// rewrites changes both sides alike. +func reencodeMappings(src []byte) ([]byte, bool) { + var root yaml.Node + if err := yaml.Unmarshal(src, &root); err != nil { + return nil, false + } + out, err := encodeYAML(&root) + if err != nil { + return nil, false + } + return out, true +} + // reverseMappings returns src with the entry order of every YAML mapping // reversed, and ok=false for a source the rewrite cannot faithfully permute: one -// that does not parse, one that will not re-encode, or one carrying a duplicate -// mapping key, whose meaning depends on the order being changed. +// that does not parse, one that will not re-encode, one carrying a duplicate +// mapping key, whose meaning depends on the order being changed, or one whose +// permutation no longer parses. +// +// That last case is the rewrite's own doing rather than a fact about the +// compiler: reversing a mapping can carry an alias above the anchor it names, +// which YAML forbids. Re-parsing catches it without enumerating it, and covers +// any later ordering rule of the same kind. func reverseMappings(src []byte) ([]byte, bool) { var root yaml.Node if err := yaml.Unmarshal(src, &root); err != nil { @@ -153,6 +231,10 @@ func reverseMappings(src []byte) ([]byte, bool) { if err != nil { return nil, false } + var check yaml.Node + if err := yaml.Unmarshal(out, &check); err != nil { + return nil, false + } return out, true } diff --git a/internal/harness/order_test.go b/internal/harness/order_test.go index 0b61bef..8b37b13 100644 --- a/internal/harness/order_test.go +++ b/internal/harness/order_test.go @@ -1,6 +1,7 @@ package harness import ( + "bytes" "context" "errors" "os" @@ -75,14 +76,31 @@ func TestReverseMappings_Permutes(t *testing.T) { // TestReverseMappings_AliasIsNotFollowed pins that an anchored mapping is // reversed once, at its anchor, rather than once per alias pointing at it. +// +// The anchor and its use sit in a sequence, whose order the rewrite leaves +// alone, so the anchor still precedes the alias afterwards. Declared as two keys +// of one mapping they would swap, and a source whose permutation puts an alias +// above its anchor is refused outright — the case below this one. func TestReverseMappings_AliasIsNotFollowed(t *testing.T) { t.Parallel() - got, ok := reverseMappings([]byte("anchor: &a\n x: 1\n y: 2\nuse: *a\n")) + got, ok := reverseMappings([]byte("items:\n - anchor: &a\n x: 1\n y: 2\n - use: *a\n")) require.True(t, ok) - assert.Equal(t, "use: *a\nanchor: &a\n y: 2\n x: 1\n", string(got), + assert.Equal(t, "items:\n - anchor: &a\n y: 2\n x: 1\n - use: *a\n", string(got), "the anchored mapping reverses once and the alias still points at it") } +// TestReverseMappings_AliasAboveItsAnchorIsRefused pins the exclusion that +// replaced the shape the test above used to assert. Reversing the two keys puts +// the alias first, and YAML requires an anchor to be defined before it is +// referenced, so the permuted source does not parse at all. Handing that to the +// compiler reported the parse failure as order dependence. +func TestReverseMappings_AliasAboveItsAnchorIsRefused(t *testing.T) { + t.Parallel() + got, ok := reverseMappings([]byte("anchor: &a\n x: 1\n y: 2\nuse: *a\n")) + assert.False(t, ok, "a permutation that no longer parses is not a faithful one") + assert.Nil(t, got) +} + // TestReverseNode_DepthBound covers the walk's bound. No document the compiler // accepts nests this deep, so the bound exists to keep the walk terminating // rather than to reject real input. @@ -112,39 +130,90 @@ func TestOrderInvariant_UnpermutableSourcePasses(t *testing.T) { for _, tc := range []struct{ name, src string }{ {"a duplicate key excludes the source", "a: 1\na: 2\n"}, {"a source with nothing to permute", "a: 1\n"}, + {"an unparseable source excludes the baseline too", "a: [unclosed\n"}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() - detail, ok := orderInvariant(context.Background(), "spec", []byte(tc.src), &ir.Document{}) + detail, ok := orderInvariant(context.Background(), "spec", []byte(tc.src)) assert.True(t, ok, "declining to ask is not a finding") assert.Empty(t, detail) }) } } -// TestOrderInvariant_CompileFailureIsReported covers the recompile's error -// paths, which a correct compiler never produces on a source that already -// compiled once. +// TestOrderInvariant_CompileFailureIsReported covers the permuted recompile's +// error paths, which a correct compiler never produces on a source whose +// baseline already compiled. +// +// The seam answers for the baseline and fails only for the permutation. Failing +// both would exercise nothing: a baseline that will not compile means the +// re-encoding is not the source, which the oracle skips rather than reports. func TestOrderInvariant_CompileFailureIsReported(t *testing.T) { const src = "a: 1\nb: 2\n" orig := compile t.Cleanup(func() { compile = orig }) - compile = func(context.Context, string, []byte) (*ir.Document, []ir.Diagnostic, error) { - return nil, nil, errors.New("boom") + baseline, ok := reencodeMappings([]byte(src)) + require.True(t, ok) + + onlyPermutedFails := func(fail func() (*ir.Document, []ir.Diagnostic, error)) { + compile = func(_ context.Context, _ string, data []byte) (*ir.Document, []ir.Diagnostic, error) { + if bytes.Equal(data, baseline) { + return &ir.Document{}, nil, nil + } + return fail() + } } - detail, ok := orderInvariant(context.Background(), "spec", []byte(src), &ir.Document{}) + + onlyPermutedFails(func() (*ir.Document, []ir.Diagnostic, error) { + return nil, nil, errors.New("boom") + }) + detail, ok := orderInvariant(context.Background(), "spec", []byte(src)) assert.False(t, ok) assert.Contains(t, detail, "boom") - compile = func(context.Context, string, []byte) (*ir.Document, []ir.Diagnostic, error) { + onlyPermutedFails(func() (*ir.Document, []ir.Diagnostic, error) { return nil, nil, nil - } - detail, ok = orderInvariant(context.Background(), "spec", []byte(src), &ir.Document{}) + }) + detail, ok = orderInvariant(context.Background(), "spec", []byte(src)) assert.False(t, ok) assert.Contains(t, detail, "no document") } +// TestOrderInvariant_UncompilableBaselineIsNotAFinding pins the skip the test +// above depends on: when the re-encoded source yields no document, the oracle has +// no faithful baseline to ask its question of, so it declines rather than +// reporting the permutation. +// +// Both ways of yielding none are driven, because they are separate branches and +// a compiler reaches them for different reasons — an I/O or programmer fault +// against a source it declines to lower at all. +func TestOrderInvariant_UncompilableBaselineIsNotAFinding(t *testing.T) { + tests := []struct { + name string + fail func() (*ir.Document, []ir.Diagnostic, error) + }{ + {"the baseline errors", func() (*ir.Document, []ir.Diagnostic, error) { + return nil, nil, errors.New("boom") + }}, + {"the baseline compiles to no document", func() (*ir.Document, []ir.Diagnostic, error) { + return nil, nil, nil + }}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + orig := compile + t.Cleanup(func() { compile = orig }) + compile = func(context.Context, string, []byte) (*ir.Document, []ir.Diagnostic, error) { + return tc.fail() + } + detail, ok := orderInvariant(context.Background(), "spec", []byte("a: 1\nb: 2\n")) + assert.True(t, ok, "no baseline is not a finding about the compiler") + assert.Empty(t, detail) + }) + } +} + // TestDiffOrderInvariants_ReportsEachChannel drives the three ways a permutation // can differ, so each message is exercised rather than only the first. func TestDiffOrderInvariants_ReportsEachChannel(t *testing.T) { @@ -214,13 +283,18 @@ func TestDiffOrderInvariants_ReorderedCollectionsAreNotAFinding(t *testing.T) { // // It asserts the oracle actually asked its question of most of the corpus, not // merely that it ran. +// +// Both arms are guarded, because either can go quiet on its own. The rewrite +// declines a source it cannot faithfully permute, and the baseline declines one +// whose re-encoding will not compile — a skip with no diagnostic behind it, so +// nothing else would notice it spreading. func TestOrderInvariant_ReachesTheCorpus(t *testing.T) { t.Parallel() const dir = "../../testdata/conformance/openapi" entries, err := os.ReadDir(dir) require.NoError(t, err) - var specs, permuted int + var specs, permuted, baselined int for _, e := range entries { if e.IsDir() || !strings.HasSuffix(e.Name(), ".yaml") { continue @@ -228,14 +302,68 @@ func TestOrderInvariant_ReachesTheCorpus(t *testing.T) { specs++ data, readErr := os.ReadFile(filepath.Join(dir, e.Name())) require.NoError(t, readErr) + + baseline, ok := reencodeMappings(data) + if !ok { + continue + } + if doc, _, compileErr := compile(t.Context(), e.Name(), baseline); compileErr == nil && doc != nil { + baselined++ + } rewritten, ok := reverseMappings(data) - if ok && string(rewritten) != string(data) { + if ok && string(rewritten) != string(baseline) { permuted++ } } require.Positive(t, specs, "found no conformance specs to permute") assert.Equal(t, specs, permuted, "every conformance spec must be permutable, or the oracle is silently skipping it") + assert.Equal(t, specs, baselined, + "every conformance spec's re-encoding must still compile, or the oracle has no baseline to compare against") +} + +// TestOrderInvariant_PermutationArtifactsAreNotFindings covers the sources whose +// permutation is not meaning-preserving for a reason reverseMappings does not +// exclude. Each compiles cleanly, and each differs between the two orders because +// of the rewrite rather than because of a lowering, so reporting one is a false +// finding about the compiler. +func TestOrderInvariant_PermutationArtifactsAreNotFindings(t *testing.T) { + t.Parallel() + tests := []struct { + name string + src string + }{ + { + // yaml.Marshal re-emits the flow mapping "{A}" — whose value is an + // implicit null — as "{A: ''}", so the second compile reads an empty + // string where the first read null. Block style keeps the null. + name: "flow-style implicit null", + src: "openapi: 3.0.0\ninfo: {title: 0, version: 0}\n" + + "components:\n schemas:\n 0:\n allOf:\n - {A}\n", + }, + { + // Reversed, the alias precedes the anchor it names, which YAML forbids: + // the permuted source does not parse at all. + name: "an alias reordered above its anchor", + src: "openapi: 3.0.0\ninfo: {title: 0, version: 0}\n0: &m\n1: *m\n", + }, + { + // Provenance.Pointer holds line:col for a reference-resolution failure, + // and a permutation moves the offending node to a different line. + name: "a diagnostic located by line and column", + src: "openapi: 3.0.0\ninfo: {title: 0, version: 0}\npaths:\n 0:\n 0:\n" + + " responses:\n 0:\n description: 0\n" + + " callbacks:\n 0:\n 0:\n description:\n", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + res := Check(context.Background(), tc.name+".yaml", []byte(tc.src)) + assert.NotEqual(t, OutcomeOrderDependent, res.Outcome, + "the permutation changed the document's meaning, so this is not a finding: %s", res.Detail) + }) + } } // yamlMapping returns a mapping node for the depth-bound test. @@ -261,6 +389,47 @@ func TestReverseMappings_EncodeFailureIsRefused(t *testing.T) { assert.Nil(t, got) } +// TestReencodeMappings_EncodeFailureIsRefused drives the defensive re-encode +// path on the baseline arm, the counterpart of the reverseMappings case above. +func TestReencodeMappings_EncodeFailureIsRefused(t *testing.T) { + orig := encodeYAML + t.Cleanup(func() { encodeYAML = orig }) + encodeYAML = func(any) ([]byte, error) { return nil, errors.New("boom") } + + got, ok := reencodeMappings([]byte("a: 1\nb: 2\n")) + assert.False(t, ok, "a source that will not re-encode is excluded, not reported") + assert.Nil(t, got) +} + +// TestIsSourcePosition_ClassifiesPointers covers the split diagnosticSet turns +// on: a source position moves with the source and must not identify a finding, +// while every other pointer spelling — a JSON pointer, an IR-space ID — is +// stable and must keep identifying one. +func TestIsSourcePosition_ClassifiesPointers(t *testing.T) { + t.Parallel() + tests := []struct { + name string + pointer string + want bool + }{ + {"a line and column", "11:21", true}, + {"the first position", "0:0", true}, + {"empty", "", false}, + {"no line", ":21", false}, + {"no column", "11:", false}, + {"a non-numeric half", "a:1", false}, + {"a third segment", "1:2:3", false}, + {"a JSON pointer", "/components/schemas/A", false}, + {"an IR-space id", "t/openapi/components/schemas/A", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, isSourcePosition(tc.pointer)) + }) + } +} + // TestCheck_OrderDependentOutcome pins that Check classifies an order-dependent // compiler as such rather than folding it into another oracle. The seam returns // a different registry for the permuted bytes, which is what a lowering that