Merge branch 'trace-element-extractor-values' into pr-73-followups

internal/runner/element_extractor_trace_test.go

@@ -0,0 +1,121 @@
+package runner
+
+import (
+	"bufio"
+	"context"
+	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/priyanshujain/sanderling/internal/trace"
+)
+
+// elementExtractorSpec reads a live ax element, the shape every field and
+// button in examples/folio/sanderling/spec.ts is extracted with. The property
+// is false the moment the field is on screen, so the run records a witness
+// whose only interesting content is that element.
+const elementExtractorSpec = `
+import { actions, always, extract } from "@sanderling/spec";
+const amountField = extract("amountField", s => s.ax.find({ "resource-id": "TxnAmountField" }));
+globalThis.properties = {
+  noAmountField: always(() => amountField.current === undefined),
+};
+globalThis.actions = actions(() => []);
+`
+
+const amountFieldTreeJSON = `{
+  "attributes": {"resource-id": "root", "bounds": "[0,0,400,800]"},
+  "enabled": true,
+  "children": [
+    {"attributes": {"resource-id": "TxnAmountField", "text": "199", "bounds": "[0,100,400,160]"},
+     "editable": true, "enabled": true, "children": []}
+  ]
+}`
+
+// TestRunner_TraceRecordsElementValuedExtractors is the guard on the artifact a
+// person opens to decide whether a conviction is real. An element-valued
+// extractor used to reach the trace as null on the goja hosts (ios, android):
+// its exported value carries the element's find/findAll host functions, which
+// json.Marshal refuses, so the encoding failed and both the per-step diff and
+// the witness recorded nothing. A witness that reads null for the field the
+// property fired on describes a state the property could not have fired in,
+// which is worse than a blank.
+func TestRunner_TraceRecordsElementValuedExtractors(t *testing.T) {
+	state := newHarnessWithSpec(t, elementExtractorSpec)
+	state.mock.HierarchyJSON = amountFieldTreeJSON
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+	summary, err := Run(ctx, Options{
+		Duration:    time.Hour,
+		IdleTimeout: 20 * time.Millisecond,
+		MaxSteps:    2,
+		Driver:      state.mock,
+		Verifier:    state.verifier,
+		TraceWriter: state.writer,
+	})
+	if err != nil {
+		t.Fatalf("Run: %v", err)
+	}
+	if !containsProperty(summary.Violations, "noAmountField") {
+		t.Fatalf("noAmountField did not violate, so the element never reached a predicate: %v",
+			summary.Violations)
+	}
+
+	file, err := os.Open(filepath.Join(state.writer.Directory(), "trace.jsonl"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer file.Close()
+
+	type traceLine struct {
+		Step             int                              `json:"step"`
+		ExtractorChanges map[string]trace.ExtractorChange `json:"extractor_changes"`
+		Witnesses        map[string]trace.Witness         `json:"witnesses"`
+	}
+	changes, witnesses := 0, 0
+	scanner := bufio.NewScanner(file)
+	scanner.Buffer(make([]byte, 0, 64*1024), 8*1024*1024)
+	for scanner.Scan() {
+		var line traceLine
+		if err := json.Unmarshal(scanner.Bytes(), &line); err != nil {
+			t.Fatalf("trace line decode: %v", err)
+		}
+		if change, ok := line.ExtractorChanges["amountField"]; ok {
+			changes++
+			assertAmountField(t, fmt.Sprintf("step %d extractor_changes", line.Step), change.Curr)
+		}
+		for name, witness := range line.Witnesses {
+			witnesses++
+			assertAmountField(t, fmt.Sprintf("step %d %s witness", line.Step, name),
+				witness.Extractors["amountField"])
+		}
+	}
+	if err := scanner.Err(); err != nil {
+		t.Fatalf("scan trace: %v", err)
+	}
+	if changes == 0 {
+		t.Error("amountField never appears in extractor_changes; the element the run read is not in the trace")
+	}
+	if witnesses == 0 {
+		t.Error("no witness reached the trace; nothing was compared")
+	}
+}
+
+// assertAmountField reads the recorded element the way a person opening the
+// trace would: the field's text is the number the property was judged on.
+func assertAmountField(t *testing.T, where string, recorded json.RawMessage) {
+	t.Helper()
+	var element struct {
+		Text string `json:"text"`
+	}
+	if err := json.Unmarshal(recorded, &element); err != nil {
+		t.Fatalf("%s: decode %s: %v", where, recorded, err)
+	}
+	if element.Text != "199" {
+		t.Errorf("%s: recorded element is %s, want its text to read 199", where, recorded)
+	}
+}

internal/verifier/extractor_encoding_test.go

@@ -0,0 +1,179 @@
+package verifier
+
+import (
+	"bytes"
+	"encoding/json"
+	"testing"
+)
+
+const elementTreeJSON = `{
+  "attributes": {"resource-id": "root", "bounds": "[0,0,400,800]"},
+  "enabled": true,
+  "children": [
+    {"attributes": {"resource-id": "TxnAmountField", "text": "199", "bounds": "[0,100,400,160]"},
+     "editable": true, "enabled": true, "children": []}
+  ]
+}`
+
+const elementExtractorSpec = `
+const field = __sanderling__.extract(state => state.ax.find({ "resource-id": "TxnAmountField" }), "field");
+globalThis.properties = {};
+`
+
+// canonicalElement is the trace's record of one ax element, written out in the
+// key order encoding/json emits. It is the contract both hosts owe the replay
+// UI: an element the reader can read, with no host-function members and nothing
+// dropped. Keys the two hosts disagree on (a DOM has no `checked`, a native
+// tree has no `dataset`) are each host's own business; the ENCODING is not.
+const canonicalElement = `{
+	"__sanderlingSelector": "resource-id:TxnAmountField",
+	"attrs": {
+		"bounds": "[0,100,400,160]",
+		"editable": "true",
+		"enabled": "true",
+		"resource-id": "TxnAmountField",
+		"text": "199"
+	},
+	"bounds": {"bottom": 160, "left": 0, "right": 400, "top": 100},
+	"checked": false,
+	"class": "",
+	"clickable": false,
+	"desc": "",
+	"editable": true,
+	"enabled": true,
+	"focused": false,
+	"id": "TxnAmountField",
+	"selected": false,
+	"text": "199",
+	"x": 200,
+	"y": 130
+}`
+
+// TestExtractorEncoding_ElementIsIdenticalOnBothHosts holds the two extractor
+// paths to one encoding of one element. The goja hosts (ios, android) run the
+// getter in-process and encode the value it returned; the web host runs it in
+// V8 and injects the page's reading through OverrideExtractorValues. A reader
+// opening a trace does not know which host wrote it, so the same element has to
+// land as the same bytes either way.
+//
+// The goja side used to write null here: an ax element carries find/findAll as
+// host functions and json.Marshal refuses the whole object over them.
+func TestExtractorEncoding_ElementIsIdenticalOnBothHosts(t *testing.T) {
+	want := compactJSON(t, canonicalElement)
+
+	native := newVerifier(t)
+	mustLoad(t, native, elementExtractorSpec)
+	pushTree(t, native, elementTreeJSON)
+	fromGoja := string(native.extractors[0].curr)
+	if fromGoja != want {
+		t.Errorf("goja host encoded the element as\n %s\nwant\n %s", fromGoja, want)
+	}
+
+	web := newVerifier(t)
+	mustLoad(t, web, elementExtractorSpec)
+	if err := web.PushSnapshot(SnapshotInput{}); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := web.OverrideExtractorValues(map[int]json.RawMessage{0: json.RawMessage(want)}); err != nil {
+		t.Fatal(err)
+	}
+	fromWeb := string(web.extractors[0].curr)
+	if fromWeb != fromGoja {
+		t.Errorf("the same element reaches the trace as\n %s\non the web host and\n %s\non goja",
+			fromWeb, fromGoja)
+	}
+}
+
+// TestExtractorEncoding_MirrorsTheWebSanitizeRule pins the goja host to the
+// rule the web host applies before a reading leaves the page (sanitize in
+// pkg/spec/src/web-runtime.ts, asserted there by the "sanitize ..." tests in
+// pkg/spec/test/web-runtime.test.ts). Two hosts encoding one value two ways is
+// the same defect as encoding it not at all: the reader cannot line the traces
+// up.
+func TestExtractorEncoding_MirrorsTheWebSanitizeRule(t *testing.T) {
+	for _, test := range []struct {
+		name       string
+		expression string
+		want       string
+	}{
+		{
+			name:       "function-valued properties are dropped",
+			expression: `({ keep: 1, fn: () => 7 })`,
+			want:       `{"keep":1}`,
+		},
+		{
+			name:       "a top-level function is not a value",
+			expression: `(() => 7)`,
+			want:       `null`,
+		},
+		{
+			name:       "a self-referential cycle breaks instead of overflowing",
+			expression: `(() => { const a = { name: "root" }; a.self = a; return a; })()`,
+			want:       `{"name":"root","self":null}`,
+		},
+		{
+			name:       "arrays and nested plain values are preserved",
+			expression: `({ items: [1, "two", { ok: true }] })`,
+			want:       `{"items":[1,"two",{"ok":true}]}`,
+		},
+		{
+			name:       "a non-finite number is not a value",
+			expression: `Number("nope")`,
+			want:       `null`,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			if got := encodeSpecValue(t, test.expression); got != test.want {
+				t.Errorf("encoded as %s, want %s", got, test.want)
+			}
+		})
+	}
+}
+
+// TestExtractorEncoding_BoundsRecursionPastTheDepthLimit mirrors the web host's
+// depth cap. state.ax hands out no cyclic element, but a spec returning a value
+// it built itself can nest without end, and a walk with no bound takes the run
+// down with a stack overflow.
+func TestExtractorEncoding_BoundsRecursionPastTheDepthLimit(t *testing.T) {
+	encoded := encodeSpecValue(t, `(() => {
+		let deep = { leaf: true };
+		for (let i = 0; i < 40; i++) deep = { next: deep };
+		return deep;
+	})()`)
+
+	var node any
+	if err := json.Unmarshal([]byte(encoded), &node); err != nil {
+		t.Fatalf("decode %s: %v", encoded, err)
+	}
+	for depth := 0; depth < recordableMaxDepth; depth++ {
+		object, ok := node.(map[string]any)
+		if !ok {
+			t.Fatalf("depth %d: recursion stopped early at %v", depth, node)
+		}
+		node = object["next"]
+	}
+	if node != nil {
+		t.Errorf("depth %d is %v, want null", recordableMaxDepth, node)
+	}
+}
+
+// encodeSpecValue returns what the trace records for an extractor whose getter
+// returned the given expression.
+func encodeSpecValue(t *testing.T, expression string) string {
+	t.Helper()
+	verifier := newVerifier(t)
+	mustLoad(t, verifier, "__sanderling__.extract(state => "+expression+", \"value\");\nglobalThis.properties = {};")
+	if err := verifier.PushSnapshot(SnapshotInput{}); err != nil {
+		t.Fatal(err)
+	}
+	return string(verifier.extractors[0].curr)
+}
+
+func compactJSON(t *testing.T, source string) string {
+	t.Helper()
+	var compact bytes.Buffer
+	if err := json.Compact(&compact, []byte(source)); err != nil {
+		t.Fatal(err)
+	}
+	return compact.String()
+}

internal/verifier/worker.go

@@ -7,6 +7,8 @@ import (
 	"errors"
 	"fmt"
 	"maps"
+	"math"
+	"reflect"
 	"sort"
 	"time"
 
@@ -356,21 +358,76 @@ func (v *Verifier) runExtractor(extractor *extractorState, state goja.Value) (go
 }
 
 // encodeExtractorValue produces a stable JSON encoding of an extractor's
-// current value for diff comparison. goja values that don't survive Export
+// current value for diff comparison. Values that still don't survive encoding
-// (e.g. wrapped host functions) yield nil; callers treat nil as "unknown" and
+// yield nil; callers treat nil as "unknown" and emit no diff entry.
-// emit no diff entry.
 func encodeExtractorValue(value goja.Value) []byte {
 	if value == nil || goja.IsUndefined(value) || goja.IsNull(value) {
 		return []byte("null")
 	}
-	exported := value.Export()
+	body, err := json.Marshal(recordableValue(value.Export(), 0, map[uintptr]bool{}))
-	body, err := json.Marshal(exported)
 	if err != nil {
 		return nil
 	}
 	return body
 }
 
+// recordableMaxDepth mirrors SANITIZE_MAX_DEPTH in pkg/spec/src/web-runtime.ts.
+const recordableMaxDepth = 32
+
+// recordableValue applies the web host's sanitize rule (web-runtime.ts) to an
+// exported goja value: function members are dropped, a cycle or a branch past
+// the depth cap becomes null, and a non-finite number becomes null. One rule on
+// both hosts is what lets the replay UI render a trace without the reader
+// having to know which host produced it. An ax element carries its find and
+// findAll host functions, and json.Marshal rejects the whole element over them,
+// so without this an element-valued extractor reached the trace as null.
+func recordableValue(value any, depth int, seen map[uintptr]bool) any {
+	switch typed := value.(type) {
+	case map[string]any:
+		address := reflect.ValueOf(typed).Pointer()
+		if depth >= recordableMaxDepth || seen[address] {
+			return nil
+		}
+		seen[address] = true
+		members := make(map[string]any, len(typed))
+		for key, member := range typed {
+			if reflect.ValueOf(member).Kind() == reflect.Func {
+				continue
+			}
+			members[key] = recordableValue(member, depth+1, seen)
+		}
+		return members
+	case []any:
+		if depth >= recordableMaxDepth {
+			return nil
+		}
+		// Every zero-length allocation shares one address, so tracking an empty
+		// array would identify it as every other empty array. It cannot close a
+		// cycle either way.
+		if len(typed) > 0 {
+			address := reflect.ValueOf(typed).Pointer()
+			if seen[address] {
+				return nil
+			}
+			seen[address] = true
+		}
+		members := make([]any, len(typed))
+		for index, member := range typed {
+			members[index] = recordableValue(member, depth+1, seen)
+		}
+		return members
+	case float64:
+		if math.IsNaN(typed) || math.IsInf(typed, 0) {
+			return nil
+		}
+		return typed
+	}
+	if reflect.ValueOf(value).Kind() == reflect.Func {
+		return nil
+	}
+	return value
+}
+
 // ChangedExtractors returns the named extractors whose value changed between
 // the prior PushSnapshot and the current one. The map is keyed by extractor
 // name; unnamed extractors (extractor_N fallback) are included so the replay