[jsonschema/schema.go] Improve error handling

[SamuelMarks]

I used the popular errcheck tool to find unhandled errors:

$ errcheck ./...                                                          
jsonschema/resolve.go:448:15:  setAnchor(s, baseInfo, anchorName, false)        
jsonschema/resolve.go:463:13:  setAnchor(s, baseInfo, s.Anchor, false)        
jsonschema/resolve.go:464:13:  setAnchor(s, baseInfo, s.DynamicAnchor, true)

Then I prepared this open-source [Apache-2.0] offline developer tool go-auto-err-handling to automatically add the if err := …; err != nil { return err } lines.

Thanks for reviewing and 🤞 merging

[karolpiotrowicz]

This does not fix what the description says it fixes, and the two calls it does change cannot fail.

bytes.Buffer.WriteByte never returns an error. From its documentation: "The returned error is always nil, but is included to match bufio.Writer's WriteByte. If the buffer becomes too large, WriteByte will panic with ErrTooLarge." So both new branches are unreachable, and the failure mode they appear to guard against is a panic that this code cannot catch.

The errcheck output quoted in the description points somewhere else. It names three unchecked setAnchor(...) calls in resolve.go, and those are still unchecked here — the diff touches schema.go instead. Those three are worth fixing: setAnchor is a closure returning a real error, duplicate anchor %q in %s, and dropping it means a duplicate $anchor is silently accepted. On this branch:

func TestDuplicateAnchor(t *testing.T) {
	doc := `{
	  "$schema": "https://json-schema.org/draft/2020-12/schema",
	  "$defs": {
	    "a": {"$anchor": "dup", "type": "string"},
	    "b": {"$anchor": "dup", "type": "integer"}
	  },
	  "$ref": "#dup"
	}`
	var s jsonschema.Schema
	json.Unmarshal([]byte(doc), &s)
	r, err := s.Resolve(nil)
	t.Logf("Resolve -> err=%v", err)
	t.Logf("Validate(42) -> %v", r.Validate(42))
}

Resolve -> err=<nil>
Validate(42) -> validating root: validating /$defs/a: type: 42 has type "integer", want "string"

The schema is rejected as malformed by other validators, and here the first anchor silently wins. That is pre-existing rather than something this PR caused, and it is the thing the quoted tool output actually found.

It is also uneven within its own function. orderedProperties.MarshalJSON makes six buffered writes. This checks two of them and leaves the buf.WriteByte(','), two buf.Write(...) and buf.WriteByte(':') calls between them untouched, all with the same always-nil contract.

Build and the full suite pass, and behaviour is unchanged — the added branches are inert. So nothing here is dangerous, it just is not worth carrying. Redirecting this at the three setAnchor calls would make it a genuinely useful change.

[SamuelMarks]

@karolpiotrowicz Thanks for reviewing. I've made some edits to my repo and see updated patch.

FWIW: Here is how you can replicate the patch:

# https://github.com/kisielk/errcheck the popular error checking linter
$ errcheck ./...
jsonschema/resolve.go:448:15:	setAnchor(s, baseInfo, anchorName, false)
jsonschema/resolve.go:463:13:	setAnchor(s, baseInfo, s.Anchor, false)
jsonschema/resolve.go:464:13:	setAnchor(s, baseInfo, s.DynamicAnchor, true)

# https://github.com/SamuelMarks/go-auto-err-handling tries to 
# faithfully reimplement errcheck, then has a Decorated Syntax Tree (DST)
# approach to automatically resolving the issues the linting phase finds
$ go-auto-err-handling ./...        
2026/09/11 01:12:08 Starting analysis on paths: [./...]
2026/09/11 01:12:08 Active Levels: Preexisting=true, ReturnTypeChanges=true, ThirdParty=true
2026/09/11 01:12:08 [1/5] Loading packages...
2026/09/11 01:12:08 Found 6 unhandled errors.
2026/09/11 01:12:08 [2/5] Loading packages...
2026/09/11 01:12:08 Codebase is stable.

[karolpiotrowicz]

Two defects here block, and they need separate edits: the return added at line 466 and the one at line 448. I've put the detail on each line. The short version is that resolve is a recursive tree walker and its child loop is at line 469, after both new returns, so returning setAnchor's result stops the walk on the success path instead of propagating an error. Both lines have to stop returning when there is no error before this can merge, and they need fixing separately — the two branches are on mutually exclusive drafts, so correcting one does not reach the other. Nothing else in the change needs to move.

The repository's own suite shows it. go test -count=1 ./... passes on the base (794ce5e4) and fails on this head (696de2fc) with a segmentation fault out of the exported Schema.Resolve, plus TestResolveURIs reporting empty anchor maps and two of five expected $id keys. GitHub's checks won't have told you any of this — the fork-approval gate is still sitting at action_required with zero runs, so no test job has ever run against this branch.

Beyond the crash, two quieter consequences follow from the same truncated walk, and both are fail-open rather than fail-closed. A $ref naming an $id that the document itself defines is now handed to ResolveOptions.Loader instead of resolving in place, and whatever the loader returns silently replaces the intended subschema. And malformed nested $ids that the base rejected now resolve without error.

The three unchecked errors you targeted are real. I ran errcheck ./... against the base myself rather than going from the pasted output, and it names exactly those three call sites. Your fix at lines 463-465 is the right shape. I applied that same form to the other two call sites and changed nothing else: the full suite goes green, errcheck stays at zero, and the duplicate anchor error that motivated this change actually surfaces, which it does not do as the branch currently stands.

Reproduction — base, this head, and the two-token variant (go test -run 'TestRepro' -v ./jsonschema/)

package jsonschema_test

import (
	"encoding/json"
	"net/url"
	"testing"

	"github.com/google/jsonschema-go/jsonschema"
)

// The commonest idiom in the format: a $ref into $defs.
func TestReproRefIntoDefs(t *testing.T) {
	var s jsonschema.Schema
	json.Unmarshal([]byte(`{
	  "$defs": {"pos": {"type": "integer", "minimum": 0}},
	  "properties": {"n": {"$ref": "#/$defs/pos"}}
	}`), &s)
	defer func() {
		if r := recover(); r != nil {
			t.Fatalf("Schema.Resolve panicked: %v", r)
		}
	}()
	r, err := s.Resolve(nil)
	if err != nil {
		t.Fatalf("Resolve: %v", err)
	}
	if err := r.Validate(map[string]any{"n": -5}); err == nil {
		t.Error("-5 accepted against minimum:0")
	}
}

// An in-document $id must not reach the loader.
type countingLoader struct{ calls []string }

func (c *countingLoader) load(u *url.URL) (*jsonschema.Schema, error) {
	c.calls = append(c.calls, u.String())
	return &jsonschema.Schema{}, nil
}

func TestReproInDocumentIDStaysLocal(t *testing.T) {
	var s jsonschema.Schema
	json.Unmarshal([]byte(`{
	  "$id": "https://example.test/root",
	  "$ref": "https://example.test/sub",
	  "$defs": {"s": {"$id": "https://example.test/sub", "type": "string", "maxLength": 3}}
	}`), &s)
	cl := &countingLoader{}
	r, err := s.Resolve(&jsonschema.ResolveOptions{Loader: cl.load})
	if err != nil {
		t.Fatalf("Resolve: %v", err)
	}
	if len(cl.calls) != 0 {
		t.Errorf("loader called for an in-document $id: %v", cl.calls)
	}
	if err := r.Validate(12345); err == nil {
		t.Error("12345 accepted against type:string")
	}
}

// The duplicate anchor this change set out to surface.
func TestReproDuplicateAnchor(t *testing.T) {
	var s jsonschema.Schema
	json.Unmarshal([]byte(`{
	  "$defs": {
	    "a": {"$anchor": "dup", "type": "string"},
	    "b": {"$anchor": "dup", "type": "integer"}
	  }
	}`), &s)
	_, err := s.Resolve(nil)
	t.Logf("Resolve -> %v", err)
}

	base	this head	with both returns unwrapped
TestReproRefIntoDefs	passes	panics, nil pointer dereference	passes
TestReproInDocumentIDStaysLocal	passes, 0 loader calls	1 loader call, 12345 accepted	passes, 0 loader calls
TestReproDuplicateAnchor	<nil> (the pre-existing bug)	<nil> (still silently accepted)	duplicate anchor "dup"
go test -count=1 ./...	ok	FAIL, SIGSEGV	ok

Worth knowing while you're in here: the draft-7 branch is executed by the existing corpus, but only in a shape where the early return happens to be harmless. Every fragment-$id fixture in testdata/draft7/ref.json (430, 457, 487, 831) and both of them in testdata/remotes/draft7/ is a childless leaf, and draft07_test.go has no $id in it at all. So a change containing only that draft-7 hunk would have shipped green. That gap predates this PR, and a draft-7 case whose fragment-$id schema has children would close it.

One small thing: the title says jsonschema/schema.go, but the diff is entirely in jsonschema/resolve.go.

[karolpiotrowicz]

This returns out of resolve whether or not setAnchor failed. setAnchor returns nil when the anchor name is empty, so on any schema without a $dynamicAnchor — most of them — it returns nil and resolve exits right here, three lines before the for c := range s.children() loop.

detectDraft falls back to draft2020 when $schema is absent or unrecognised, so this branch is the default path rather than an edge case. resolve is first called on the root, so the walk now ends at depth 1 and every subschema keeps info.base == nil. resolveRefs still visits those subschemas, because it walks the tree through Schema.all() rather than through this function, and resolveRef then dereferences the nil base. That is the segmentation fault.

The invariant to hold on to: every schema reachable from root.all() gets info.base, and this function visits every child on every path out of it. Propagating an error must not change control flow when there is no error.

[karolpiotrowicz]

Same shape as line 466, and it needs its own edit — fixing 466 does not reach this, because the two branches are on mutually exclusive drafts. This one also skips info.base = base at line 461.

I checked the objection that a schema reaching this line cannot itself carry a $ref, which is true because of the ignore guard at line 434. It does not save the branch, because the crash comes from a child. This document resolves and validates on the base and panics on this head, and its $schema is draft-07 so line 466 never executes — the branch is isolated:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "outer": {
      "$id": "#outer",
      "properties": {"p": {"$ref": "#/definitions/B"}}
    },
    "B": {"type": "string"}
  },
  "allOf": [{"$ref": "#/definitions/outer"}]
}

Nothing in the existing corpus covers this: every fragment-$id fixture under testdata/draft7/ is a childless leaf, so a change containing only this hunk would have gone green.