feat(schema): make adding a key a compatible change

_plans/054_open-json-schema.md

@@ -0,0 +1,109 @@
+# 054: adding a key is a compatible schema change
+
+Answers #200. v0.1.0 is released and `--json` has consumers:
+`mozilla/markfluence-action` exposes the envelope to later workflow steps as
+its `results-json` output. `docs/json-output.md` says a change that breaks
+compatibility increases `schema_version`, but not what breaks it. And the
+published schema sets `additionalProperties: false` on every object, so a
+consumer validating against a copy of the v0.1.0 schema rejects any new key:
+adding a field is breaking in practice. #10 needs one (`update`'s `moved`), so
+this comes first.
+
+## What the survey established
+
+- `additionalProperties: false` appears 48 times, always on a node with
+  `"type": "object"` and `properties`, and every such node has it. So "close
+  every typed object that lists properties" reproduces the current schema
+  exactly. The `if`/`then` branches carry no `type`, and neither does the
+  envelope's per-command `then`, so neither is closed; they constrain
+  `results` and `summary`, which the envelope's own `properties` declare.
+- **Opening the objects breaks three unions.** Eight `results.items` are a
+  `oneOf` of a command's result and `singleOpFailure`. Five tell them apart by
+  `ok: const true` against `ok: const false`. The other three
+  (`exportResult`, `attachmentUploadResult`, `attachmentDownloadResult`)
+  carry `ok: boolean` and rely on closed objects to be exclusive.
+  `exportResult` declares every key `singleOpFailure` requires, so with open
+  objects a failed export row matches both, and `oneOf` fails. The two
+  attachment results lack `page_id` only, so they would become ambiguous the
+  day one gains it.
+
+## Decisions
+
+**D1. The compatibility rule, in `docs/json-output.md`.**
+
+- *Compatible*, and `schema_version` stays: a new key on the envelope, a
+  result, a summary, or the error object; a new value in an enum the schema
+  documents as open (none today); loosening a constraint.
+- *Breaking*, and `schema_version` goes up: removing or renaming a key;
+  changing a key's type, or what it means; making a key nullable that was
+  not; a new value in a closed enum (`code`, `status`, `command`...).
+- Consumers must ignore keys they do not know, and should validate against
+  the schema that `markfluence schema` prints, which is the running binary's.
+
+A new `command` enum value counts as breaking for the rule's sake, since a
+consumer switching on `command` meets a value it has no case for. In practice
+a new command is new output nobody consumed before; that is judged when it
+happens, not here.
+
+**D2. The published schema is open.** Remove every `additionalProperties:
+false` from `schema/json-output/v1.json`. Every document valid before stays
+valid, so this is itself compatible.
+
+**D3. The unions become `anyOf`.** All eight result-or-failure unions change
+from `oneOf` to `anyOf`. `anyOf` is looser than `oneOf`, so this is
+compatible too, and it is what an open schema needs: "matches at least one
+shape" is the claim a consumer can rely on. The scalar unions
+(`stringOrNull`, `codeOrNull`) stay `oneOf`; their branches cannot overlap.
+
+**D4. The tests close it again.** `internal/schematest` compiles a *closed*
+copy: it walks the embedded schema and sets `additionalProperties: false` on
+every node with `"type": "object"` and `properties` that does not already say
+otherwise. `ValidateEnvelope`/`ValidateError` validate against that, so the
+drift guard is unchanged: a key the code emits and the schema does not list
+still fails. The closing lives in one exported function (`Closed`) so a test
+can check it directly.
+
+**D5. The embedded schema is still what ships.** `markfluence schema` prints
+the open document, byte for byte the file. The closing happens in memory, in
+test code only.
+
+## Implementation
+
+- Before editing the schema, a one-off check: strip every
+  `additionalProperties: false` from the current file, run `Closed` on the
+  result, and compare with the original as parsed JSON. They must be equal.
+  That is the proof that D4 restores exactly what D2 removes; the
+  permanent tests below keep it true.
+- Then the schema edit, done by a script (strip the key; `oneOf` → `anyOf`
+  in the eight unions), with the formatting kept.
+
+## Files
+
+| file | change |
+|---|---|
+| `schema/json-output/v1.json` | D2, D3; the top-level `description` says the schema is open and why |
+| `internal/schematest/schematest.go` | D4: `Closed`, compile the closed copy; package doc |
+| `internal/schematest/document_test.go` (or a new test file) | tests below |
+| `docs/json-output.md` | D1 |
+| `CLAUDE.md` | the `schematest` and `schema/` bullets |
+| code comments naming `additionalProperties:false` (`cmd/diff`, `cmd/search`, `cmd/userfind`, `cmd/spaceinfo`, `internal/jsonout`) | say "the closed schema the tests validate against" |
+
+## Tests
+
+- The published schema contains no `additionalProperties: false`.
+- `Closed` closes every typed object that lists properties, and nothing else
+  (the `if`/`then` nodes stay open).
+- A document with an extra key on a result fails `ValidateEnvelope` and passes
+  the published schema. That pins both halves: the drift guard, and the
+  consumer promise.
+- A failed export row validates against the published schema (the union
+  ambiguity D3 fixes). It fails today with objects opened and `oneOf` kept.
+- Every existing conformance test passes unchanged.
+
+## Not in scope
+
+- **A test comparing against the released schema** (fetching v0.1.0's file
+  and checking no key was removed or retyped). `required` already catches a
+  removed key from our side; a check across releases waits until a breaking
+  change slips through.
+- **A `schema_version` bump.** Nothing here breaks a consumer.

cmd/diff/json.go

@@ -11,8 +11,8 @@ import (
 // diffResult is the --json result for one file.
 //
 // Every field is on this struct and nothing uses omitempty, so every one always
-// marshals and the schema's additionalProperties:false / required catch an
-// added, renamed or removed field whatever a fixture sets.
+// marshals and the closed schema the tests validate against (schematest.Closed)
+// and required catch an added, renamed or removed field whatever a fixture sets.
 type diffResult struct {
 	OK   bool   `json:"ok"`
 	File string `json:"file"`

cmd/schema/schema.go

@@ -18,8 +18,13 @@ var Cmd = &cobra.Command{
 		"Thus a script, a CI job, or an agent can get the contract from the binary, and\n"+
 		"not from the repository.\n\n"+
 		"The build puts the schema into the binary. It describes schema_version %d,\n"+
-		"which is the version that this binary writes. The schema command, and the tests\n"+
-		"that check real --json output, read the same copy.\n\n"+
+		"which is the version that this binary writes. Validate against this copy, and\n"+
+		"not against a copy from another release.\n\n"+
+		"The schema is open: an object can have keys that the schema does not list. A\n"+
+		"later release can add a key and keep the same schema_version, so a consumer must\n"+
+		"ignore a key that it does not know. A change that can break a consumer, such as\n"+
+		"a key that is removed or renamed, increases schema_version. docs/json-output.md\n"+
+		"has the whole rule.\n\n"+
 		"The output is the schema document itself, so --json has no effect here.",
 		jsonout.SchemaVersion),
 	Example: "  # Save the schema\n" +

cmd/search/json.go

@@ -28,8 +28,9 @@ type jsonSearchResult struct {
 
 // jsonSearchSummary is search's summary.
 //
-// basicSummary cannot be reused: it is additionalProperties:false, and both
-// fields below are load-bearing.
+// basicSummary cannot be reused: it lists neither field, the closed schema the
+// tests validate against refuses an unlisted key, and both fields below are
+// load-bearing.
 //
 // truncated is a flag rather than a count of what was dropped, because there is
 // no trustworthy count to report -- totalSize drifted 294/292/291 against 289

cmd/spaceinfo/spaceinfo_test.go

@@ -548,7 +548,7 @@ func TestUnknownSpaceIsNil(t *testing.T) {
 }
 
 // Every field always marshals, per internal/schematest's rule: no omitempty,
-// so additionalProperties:false and required catch an added or renamed one.
+// so the tests' closed schema and required catch an added or renamed one.
 func TestJSONResultAlwaysCarriesEveryField(t *testing.T) {
 	r := stub{pages: [][]string{{row("1", "current", "", 30, 30)}}}.build(t, 7)
 	blob, err := json.Marshal(r.jsonResult())

cmd/userfind/json.go

@@ -31,8 +31,9 @@ type jsonUserResult struct {
 
 // jsonUserSummary is user-find's summary.
 //
-// basicSummary cannot be reused: it is additionalProperties:false, and
-// truncated is load-bearing. It is a flag rather than a count for a sharper
+// basicSummary cannot be reused: it does not list truncated, the closed schema
+// the tests validate against refuses an unlisted key, and truncated is
+// load-bearing. It is a flag rather than a count for a sharper
 // reason than search's drift -- this route's totalSize reports the rows on the
 // current page, so it cannot describe the result set at all.
 type jsonUserSummary struct {

docs/commands/markfluence_schema.md

@@ -9,8 +9,14 @@ Thus a script, a CI job, or an agent can get the contract from the binary, and
 not from the repository.
 
 The build puts the schema into the binary. It describes schema_version 1,
-which is the version that this binary writes. The schema command, and the tests
-that check real --json output, read the same copy.
+which is the version that this binary writes. Validate against this copy, and
+not against a copy from another release.
+
+The schema is open: an object can have keys that the schema does not list. A
+later release can add a key and keep the same schema_version, so a consumer must
+ignore a key that it does not know. A change that can break a consumer, such as
+a key that is removed or renamed, increases schema_version. docs/json-output.md
+has the whole rule.
 
 The output is the schema document itself, so --json has no effect here.
 

docs/json-output.md

@@ -10,12 +10,51 @@ The JSON Schema at [`schema/json-output/v1.json`](../schema/json-output/v1.json)
 is the authoritative contract for each field. The `markfluence schema` command
 also prints it.
 
+## Compatibility
+
+`schema_version` is `1`. It increases only for a change that can break a
+consumer.
+
+These changes are compatible, and `schema_version` stays the same:
+
+- a new key on the envelope, on a result, on a summary, or on the error
+  object;
+- a constraint that is made looser;
+
+- a new command, which adds a value to the `command` enum. Only the new
+  command's own output carries that value, so no consumer of an existing
+  command sees it;
+- a new value of `field` in a `diff` result. It is a string rather than an
+  enum for this reason: a frontmatter field that markfluence learns later is
+  reported there too.
+
+These changes break compatibility, and `schema_version` increases:
+
+- a key that is removed or renamed;
+- a key whose type or meaning changes;
+- a key that can be `null` and could not before;
+- a new value in any other enum, such as `code` or a result's `status`. A
+  consumer can reasonably handle each value of those, and a new one would
+  reach a consumer of an existing command.
+
+Thus a consumer must ignore a key that it does not know. The published schema
+is open for this reason: an object can have keys that the schema does not list.
+If you validate the output, validate against the schema that
+`markfluence schema` prints, which is the schema of the binary that you run.
+A copy from an older release does not list the newer keys, but it still
+accepts them.
+
+markfluence's own tests validate against a closed copy of the schema, which
+refuses any key that the schema does not list. Thus a key cannot get into the
+output before it gets into the schema.
+
 ## Notes on the schema
 
 - **Stable for each command.** Each command always writes the same keys in the
   same shapes. An empty value is `null` or `[]`. The *set* of keys is different
   for each command. Any change that breaks compatibility increases
-  `schema_version`.
+  `schema_version`. See [Compatibility](#compatibility) for which changes
+  those are.
 - **`roots`** lists each different
   [documentation root](../README.md#the-documentation-root) that the command
   resolved, in sorted order. It is `[]` for a command that has no per-file root,

internal/jsonout/types.go

@@ -84,8 +84,8 @@ type AttachmentActionResult struct {
 // #/$defs/singleOpFailure in the schema.
 //
 // It is a struct rather than the map each command used to build inline because
-// every field then marshals unconditionally, which is what lets the schema's
-// additionalProperties:false and required catch a renamed or added key. A map
+// every field then marshals unconditionally, which is what lets the closed
+// schema the tests validate against, and required, catch a renamed or added key. A map
 // only carries the keys the caller remembered to set, so drift in one showed up
 // nowhere.
 type SingleOpFailure struct {

internal/schematest/closed_test.go

@@ -0,0 +1,125 @@
+package schematest
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/mozilla/markfluence/schema"
+	"github.com/santhosh-tekuri/jsonschema/v6"
+)
+
+func decoded(t *testing.T) any {
+	t.Helper()
+	doc, err := jsonschema.UnmarshalJSON(strings.NewReader(schema.V1))
+	if err != nil {
+		t.Fatal(err)
+	}
+	return doc
+}
+
+// TestPublishedSchemaIsOpen pins the consumer half of #200: an object the
+// published file closes is one no later release may add a key to without
+// breaking whoever validates against the copy they have. Any other
+// additionalProperties (a schema for a map's values, say) is fine.
+func TestPublishedSchemaIsOpen(t *testing.T) {
+	eachSchema(decoded(t), "#", func(path string, node map[string]any) {
+		if node["additionalProperties"] == false {
+			t.Errorf("%s sets additionalProperties: false; the published schema must stay open "+
+				"(the tests close it: see closeObjects)", path)
+		}
+	})
+}
+
+// TestEveryPropertiesNodeIsTyped keeps the closing complete. closeObjects
+// closes only a node declaring "type": "object", so a nested object written
+// without a type would be left open, and a stray key on it would pass the
+// drift guard with every test green. The one legitimate untyped node listing
+// properties is an envelope if/then branch.
+func TestEveryPropertiesNodeIsTyped(t *testing.T) {
+	eachSchema(decoded(t), "#", func(path string, node map[string]any) {
+		if _, ok := node["properties"]; !ok || isObjectSchema(node) {
+			return
+		}
+		if strings.HasPrefix(path, "#/allOf/") &&
+			(strings.HasSuffix(path, "/if") || strings.HasSuffix(path, "/then")) {
+			return
+		}
+		t.Errorf(`%s lists properties without "type": "object", so the drift guard leaves it open`, path)
+	})
+}
+
+// TestCloseObjectsClosesObjectSchemasOnly: every object schema is closed, and
+// nothing else is -- the if/then branches least of all.
+func TestCloseObjectsClosesObjectSchemasOnly(t *testing.T) {
+	doc := decoded(t)
+	closeObjects(doc)
+	closed := 0
+	eachSchema(doc, "#", func(path string, node map[string]any) {
+		ap, said := node["additionalProperties"]
+		switch {
+		case isObjectSchema(node):
+			if ap != false {
+				t.Errorf("%s: object schema not closed", path)
+			}
+			closed++
+		case said:
+			t.Errorf("%s: closed, but it is not an object schema", path)
+		}
+	})
+	if closed == 0 {
+		t.Fatal("closed nothing")
+	}
+}
+
+// TestCloseObjectsLeavesInstanceDataAlone: a const is a value, and closing an
+// object-shaped one would change what it requires.
+func TestCloseObjectsLeavesInstanceDataAlone(t *testing.T) {
+	doc := map[string]any{
+		"type":       "object",
+		"properties": map[string]any{"x": map[string]any{}},
+		"const":      map[string]any{"type": "object", "properties": map[string]any{}},
+	}
+	closeObjects(doc)
+	if doc["additionalProperties"] != false {
+		t.Error("the schema itself was not closed")
+	}
+	if _, said := doc["const"].(map[string]any)["additionalProperties"]; said {
+		t.Error("closeObjects wrote into a const")
+	}
+}
+
+// exportEnvelope is a valid export envelope whose one row is a failed export,
+// with extra added to that row.
+func exportEnvelope(extra string) []byte {
+	return []byte(`{"schema_version": 1, "markfluence_version": "dev", "command": "export",
+		"roots": [], "warnings": [],
+		"results": [{"ok": false, "page_id": "123", "title": "", "space": "", "parent": null,
+			"parent_type": null, "parent_file": null, "dry_run": false, "status": "",
+			"dest_path": null, "attachments": [], "warnings": [],
+			"error": "boom", "code": "NETWORK"` + extra + `}],
+		"summary": {"total": 1, "succeeded": 0, "failed": 1, "skipped": 0, "project_file": null}}`)
+}
+
+// TestAnUnlistedKeyIsCompatible pins both halves at once: the drift guard
+// refuses a key the schema does not list, and the published schema accepts
+// it, which is what makes adding a key a compatible change.
+func TestAnUnlistedKeyIsCompatible(t *testing.T) {
+	doc := exportEnvelope(`, "brand_new": 1`)
+	if err := check(t, compile(t, true).envelope, doc); err == nil {
+		t.Error("the closed schema accepted an unlisted key; the drift guard is off")
+	}
+	if err := check(t, compile(t, false).envelope, doc); err != nil {
+		t.Errorf("the published schema refused an unlisted key: %v", err)
+	}
+}
+
+// TestFailedExportRowIsValid is the union #200 found. A failed export row has
+// every key singleOpFailure requires, so once objects are open it matches
+// both shapes, and a oneOf refuses a row that matches two.
+func TestFailedExportRowIsValid(t *testing.T) {
+	doc := exportEnvelope("")
+	if err := check(t, compile(t, false).envelope, doc); err != nil {
+		t.Errorf("published schema: %v", err)
+	}
+	ValidateEnvelope(t, doc)
+}