diff --git a/docs/content/docs/manual/json-schemas.md b/docs/content/docs/manual/json-schemas.md index 83e71538..6e1c0170 100644 --- a/docs/content/docs/manual/json-schemas.md +++ b/docs/content/docs/manual/json-schemas.md @@ -25,11 +25,13 @@ the published file differs from the source), next to | `customizations.workharbor` in `devcontainer.json` | `https://wstein.github.io/workharbor/schemas/devcontainer-workharbor.v0-provisional.schema.json` | **Versions.** `v0-provisional` is the name of the configuration these files -map: the known structure as of 2026-10. A published file never changes. When the -configuration changes, the schema gets a new name (`v1-provisional`, later a -stable `v1`) and the old file and its URL stay, so a file that names an older -version keeps working in an editor. That the published file is never edited is -a rule of review; no test checks it against history. +map: the known structure as of 2026-10. A published file never changes once a +stable name is chosen; the provisional files may still be corrected until then +(no one relies on them yet). When the configuration changes, the schema gets a +new name (`v1-provisional`, later a stable `v1`) and the old file and its URL +stay, so a file that names an older version keeps working in an editor. That a +published file is never edited is a rule of review; no test checks it against +history. **How they are kept honest.** A Go test compares each schema recursively with the Go struct the product reads (every key at every depth on both sides, types diff --git a/internal/config/schema_test.go b/internal/config/schema_test.go index 18aee262..5b6253ff 100644 --- a/internal/config/schema_test.go +++ b/internal/config/schema_test.go @@ -91,7 +91,7 @@ func TestSchemaAcceptsGoodExamplesAndRejectsWrongOnes(t *testing.T) { "missing": {mutate(func(m map[string]any) { delete(nested(m, "roots"), "tool_store") }), `missing required key "tool_store"`}, "enum": {mutate(func(m map[string]any) { m["account"] = "private" }), "$.account: "}, "enum in a list": {mutate(func(m map[string]any) { m["repositories"].([]any)[0].(map[string]any)["workflow"] = "yolo" }), "$.repositories[0].workflow: "}, - "range": {mutate(func(m map[string]any) { m["preview"] = map[string]any{"first_port": 80, "last_port": 90} }), "$.preview.first_port: 80 violates minimum"}, + "range": {mutate(func(m map[string]any) { m["preview"] = map[string]any{"first_port": 80, "last_port": 90} }), "$.preview.first_port: matches none of the alternatives"}, "percent": {mutate(func(m map[string]any) { m["budgets"] = map[string]any{"soft_percent": 100} }), "$.budgets.soft_percent: 100 violates maximum"}, "list item type": {mutate(func(m map[string]any) { m["agent_allowed_tools"] = []any{"Read", 7} }), "$.agent_allowed_tools[1]: want string"}, "empty workspace": {mutate(func(m map[string]any) { nested(m, "roots")["workspaces"] = []any{} }), "fewer than 1 items"}, @@ -105,6 +105,63 @@ func TestSchemaAcceptsGoodExamplesAndRejectsWrongOnes(t *testing.T) { } } +// TestSchemaAcceptsWhatTheLoaderAccepts pins the values the loader takes that +// a stricter schema would flag: 0 means "default" or "off" for the percents and +// the preview range, and url.Parse lowercases the scheme. Each document is also +// parsed by the loader, so the schema cannot drift from it. Out-of-range values +// must keep failing in the schema. +func TestSchemaAcceptsWhatTheLoaderAccepts(t *testing.T) { + s := schematest.Load(t, configSchemaPath) + base, _ := json.Marshal(newRig(t).cfg) + with := func(edit func(m map[string]any)) []byte { + var m map[string]any + if err := json.Unmarshal(base, &m); err != nil { + t.Fatal(err) + } + edit(m) + raw, _ := json.Marshal(m) + return raw + } + ok := map[string]func(m map[string]any){ + "soft_percent 0": func(m map[string]any) { m["budgets"] = map[string]any{"soft_percent": 0} }, + "warn_percent 0": func(m map[string]any) { m["limits"] = map[string]any{"warn_percent": 0} }, + "preview off": func(m map[string]any) { m["preview"] = map[string]any{"first_port": 0, "last_port": 0} }, + "public_url scheme": func(m map[string]any) { m["public_url"] = "HTTPS://whr.example.ts.net" }, + "board scheme": func(m map[string]any) { + m["board"] = map[string]any{"owner": "octo", "number": 1, "public_url": "Https://whr.example.ts.net"} + }, + } + for name, edit := range ok { + t.Run("accepts "+name, func(t *testing.T) { + doc := with(edit) + if p := schematest.Validate(s, doc); len(p) != 0 { + t.Errorf("schema rejects a document the loader accepts: %v", p) + } + if _, err := Parse(doc); err != nil { + t.Errorf("loader rejects the document: %v", err) + } + }) + } + bad := map[string]struct { + edit func(m map[string]any) + want string + }{ + "soft_percent -1": {func(m map[string]any) { m["budgets"] = map[string]any{"soft_percent": -1} }, "violates minimum"}, + "warn_percent 100": {func(m map[string]any) { m["limits"] = map[string]any{"warn_percent": 100} }, "violates maximum"}, + "preview 1023": {func(m map[string]any) { m["preview"] = map[string]any{"first_port": 1023, "last_port": 2000} }, "matches none"}, + "preview 65536": {func(m map[string]any) { m["preview"] = map[string]any{"first_port": 2000, "last_port": 65536} }, "matches none"}, + "public_url http": {func(m map[string]any) { m["public_url"] = "http://whr.example.ts.net" }, "does not match"}, + } + for name, tc := range bad { + t.Run("rejects "+name, func(t *testing.T) { + got := strings.Join(schematest.Validate(s, with(tc.edit)), "\n") + if !strings.Contains(got, tc.want) { + t.Errorf("problems = %q, want one containing %q", got, tc.want) + } + }) + } +} + // TestSchemaKeyIsDataOnly pins the rule of issue #268. Parse decodes the file // with the Go struct and nothing else: the only place that ever sees the value // is Config.Schema, and no code in this package imports net/http or net, opens a diff --git a/internal/schematest/schematest.go b/internal/schematest/schematest.go index 06938ab7..ba769985 100644 --- a/internal/schematest/schematest.go +++ b/internal/schematest/schematest.go @@ -46,6 +46,7 @@ func Validate(root Schema, raw []byte) []string { if err := json.Unmarshal(raw, &doc); err != nil { return []string{"not JSON: " + err.Error()} } + checkKeywords(root, "$") var problems []string validate(root, root, doc, "$", &problems) return problems @@ -65,6 +66,38 @@ func resolve(root, s object) object { return target } +// supported lists the keywords validate evaluates; annotations are the others +// that are allowed. +var supported = []string{"type", "enum", "minimum", "maximum", "maxLength", "pattern", "minItems", "items", "anyOf", "required", "additionalProperties", "properties", "$ref"} + +// checkKeywords walks the whole schema once, whether or not a document reaches +// a subschema, and panics on any keyword that is neither supported nor an +// annotation. It follows properties, items, anyOf and $defs; a schema-valued +// additionalProperties is unsupported (only false and true are evaluated). +func checkKeywords(s object, path string) { + for k, v := range s { + if !slices.Contains(supported, k) && !slices.Contains(annotations, k) { + panic("schematest: unsupported keyword " + k + " at " + path) + } + switch k { + case "properties", "$defs": + for name, sub := range v.(object) { + checkKeywords(sub.(object), path+"."+k+"."+name) + } + case "items": + checkKeywords(v.(object), path+".items") + case "anyOf": + for i, sub := range v.([]any) { + checkKeywords(sub.(object), fmt.Sprintf("%s.anyOf[%d]", path, i)) + } + case "additionalProperties": + if _, ok := v.(bool); !ok { + panic("schematest: additionalProperties must be a boolean at " + path) + } + } + } +} + func validate(root, s object, v any, path string, out *[]string) { s = resolve(root, s) bad := func(format string, args ...any) { *out = append(*out, path+": "+fmt.Sprintf(format, args...)) } @@ -184,6 +217,7 @@ func typeMatches(want string, v any) bool { // nested objects, arrays and pointers. func CompareStruct(t testing.TB, root Schema, typ reflect.Type, extraOptional map[string]string, rawAny ...string) { t.Helper() + checkKeywords(root, "$") compare(t, root, root, typ, "$", extraOptional, rawAny) } diff --git a/internal/schematest/schematest_test.go b/internal/schematest/schematest_test.go index 92bef269..45fd702c 100644 --- a/internal/schematest/schematest_test.go +++ b/internal/schematest/schematest_test.go @@ -70,3 +70,23 @@ func (r *recorder) Helper() {} func (r *recorder) Errorf(format string, args ...any) { r.errs = append(r.errs, fmt.Sprintf(format, args...)) } + +func TestUnsupportedKeywordInUnreachedSubschemaPanics(t *testing.T) { + for name, doc := range map[string]string{ + "property": `{"type":"object","properties":{"a":{"type":"array","uniqueItems":true,"items":{"type":"string"}}}}`, + "def": `{"type":"object","$defs":{"d":{"type":"string","minLength":4}},"properties":{"a":{"$ref":"#/$defs/d"}}}`, + "anyOf": `{"type":"object","properties":{"a":{"anyOf":[{"type":"string"},{"type":"integer","multipleOf":2}]}}}`, + "items": `{"type":"object","properties":{"a":{"type":"array","items":{"type":"string","format":"uri"}}}}`, + "additional": `{"type":"object","additionalProperties":{"type":"string","minLength":1}}`, + } { + t.Run(name, func(t *testing.T) { + defer func() { + if recover() == nil { + t.Fatal("an unsupported keyword in a subschema no document reaches must panic") + } + }() + // The document reaches none of the subschemas. + Validate(schema(t, doc), []byte(`{}`)) + }) + } +} diff --git a/schemas/config.v0-provisional.schema.json b/schemas/config.v0-provisional.schema.json index a6d4454b..3c845770 100644 --- a/schemas/config.v0-provisional.schema.json +++ b/schemas/config.v0-provisional.schema.json @@ -22,7 +22,7 @@ } }, "listen": { "type": "string", "description": "Loopback address of the web UI, for example 127.0.0.1:8787." }, - "public_url": { "type": "string", "pattern": "^https://" }, + "public_url": { "type": "string", "pattern": "^[hH][tT][tT][pP][sS]://" }, "repositories": { "type": "array", "items": { @@ -105,14 +105,14 @@ "properties": { "per_run": { "$ref": "#/$defs/budgetLimit" }, "per_task": { "$ref": "#/$defs/budgetLimit" }, - "soft_percent": { "type": "integer", "minimum": 1, "maximum": 99 } + "soft_percent": { "type": "integer", "minimum": 0, "maximum": 99 } } }, "limits": { "type": "object", "additionalProperties": false, "properties": { - "warn_percent": { "type": "integer", "minimum": 1, "maximum": 99 }, + "warn_percent": { "type": "integer", "minimum": 0, "maximum": 99 }, "low_balance_usd": { "type": "number", "minimum": 0 } } }, @@ -120,8 +120,8 @@ "type": "object", "additionalProperties": false, "properties": { - "first_port": { "type": "integer", "minimum": 1024, "maximum": 65535 }, - "last_port": { "type": "integer", "minimum": 1024, "maximum": 65535 } + "first_port": { "type": "integer", "anyOf": [{ "enum": [0] }, { "minimum": 1024, "maximum": 65535 }], "description": "0 (both ports) turns previews off; otherwise a port from 1024 to 65535." }, + "last_port": { "type": "integer", "anyOf": [{ "enum": [0] }, { "minimum": 1024, "maximum": 65535 }], "description": "0 (both ports) turns previews off; otherwise a port from 1024 to 65535." } } }, "tool_profile": { "type": "string" }, @@ -137,7 +137,7 @@ "session_field": { "type": "string" }, "link_field": { "type": "string" }, "queue_status": { "type": "string" }, - "public_url": { "type": "string", "pattern": "^https://" } + "public_url": { "type": "string", "pattern": "^[hH][tT][tT][pP][sS]://" } } }, "ntfy": { diff --git a/scripts/docs_schema_test.go b/scripts/docs_schema_test.go index f8bba208..6bf082c4 100644 --- a/scripts/docs_schema_test.go +++ b/scripts/docs_schema_test.go @@ -24,6 +24,16 @@ var ( siteErr error ) +// TestMain removes the site this package built itself; a site given with +// -docs-site belongs to the caller and stays. +func TestMain(m *testing.M) { + code := m.Run() + if siteDir != "" { + _ = os.RemoveAll(siteDir) + } + os.Exit(code) +} + // builtSite returns the already built site of -docs-site, else builds it once // for all tests of this package. func builtSite(t *testing.T) string {