fix(engine)!: keep compiler diagnostics and exit on spec problems

README.md

@@ -120,9 +120,15 @@ Help always prints to stdout and exits `0`.
 | `-o <file>` | `compile` | Write IR JSON to `<file>` instead of stdout. |
 | `--explain <json-pointer>` | `compile` | Report what compiling produced at this source coordinate instead of writing the document. |
 
-Diagnostics print one per line as `<severity> <code> <path>#<pointer>: <message>`. Both commands
-use the same exit codes: `0` clean (and for any help request), `1` a diagnostic reached the
-`--fail-on` threshold (or the spec could not be lowered), `2` a usage or I/O error.
+Diagnostics print one per line as `<severity> <code> <location>: <message>`, where `<location>` is
+`<path>#<pointer>` for a finding in a spec file, a bare pointer for one an IR pass made about the
+document, and absent for one raised before any document existed.
+
+Both commands use the same exit codes: `0` clean (and for any help request); `1` the spec has
+problems — a diagnostic reached the `--fail-on` threshold, or it could not be lowered at all, which
+covers an undecodable file, an unrecognized or unsupported format, and a version no compiler claims;
+`2` the invocation or the filesystem was wrong — a bad flag or argument, a spec that could not be
+read, an output that could not be written. Nothing about the spec's own contents reaches `2`.
 
 ### Library
 

cmd/morphic/compile.go

@@ -239,19 +239,32 @@ func parseArgs(fs *flag.FlagSet, args []string) ([]string, error) {
 }
 
 // renderDiagnostics writes each diagnostic to w, one per line, as
-// "<severity> <code> <path>#<pointer>: <message>". This is the sole place in
-// the pipeline where diagnostics are rendered for a human.
+// "<severity> <code> <location>: <message>". This is the sole place in the
+// pipeline where diagnostics are rendered for a human.
 func renderDiagnostics(w io.Writer, res *engine.Result) {
 	for _, d := range res.Diagnostics {
-		if path := sourcePath(res.Document, d.Provenance.Source); path != "" {
-			emitf(w, "%s %s %s#%s: %s\n",
-				d.Severity, d.Code, path, d.Provenance.Pointer, d.Message)
-			continue
-		}
-		// No source file (e.g. a pass diagnostic whose pointer is an IR id): show
-		// the bare pointer rather than fabricating a location in the spec file.
-		emitf(w, "%s %s %s: %s\n", d.Severity, d.Code, d.Provenance.Pointer, d.Message)
+		emitf(w, "%s %s%s: %s\n",
+			d.Severity, d.Code, location(res.Document, d.Provenance), d.Message)
+	}
+}
+
+// location renders the "where" of a diagnostic line, leading space included:
+// " <path>#<pointer>" when the provenance resolves to a source file,
+// " <pointer>" when it names only an IR-space position (a pass diagnostic whose
+// pointer is an IR id), and nothing at all when it names neither.
+//
+// The empty case is what a diagnostic raised before any document existed
+// carries — an unrecognized spec format has no position inside a spec that was
+// never lowered — and printing nothing is the point: any location shown there
+// would be one the finding is not about.
+func location(doc *ir.Document, prov ir.Provenance) string {
+	if path := sourcePath(doc, prov.Source); path != "" {
+		return " " + path + "#" + prov.Pointer
+	}
+	if prov.Pointer != "" {
+		return " " + prov.Pointer
 	}
+	return ""
 }
 
 // sourcePath resolves a diagnostic's source index to its file path, returning

cmd/morphic/compile_test.go

@@ -39,13 +39,43 @@ func TestRun_ParseWritesIRToFile(t *testing.T) {
 	assert.True(t, bytes.HasSuffix(raw, []byte("\n")))
 }
 
-func TestRun_ParseUnknownSpecFails(t *testing.T) {
+// TestRun_SpecProblemsExitOne covers the classes of bad spec the pipeline
+// rejects before a compiler ever runs. Every one of them is a problem with the
+// file the user named, so every one is exit 1 with a coded diagnostic — the same
+// answer a bad `$ref` inside a spec the compiler does accept already gets.
+//
+// Exit 2 is reserved for a misuse of the CLI and for I/O that failed, so a CI
+// wrapper can tell "your spec is broken" from "you invoked morphic wrong". A
+// spec the tool read and understood well enough to name the problem in is
+// neither of those, and reporting it as one made the compiler's own
+// openapi/unsupported-version diagnostic unreachable from the shipped binary.
+func TestRun_SpecProblemsExitOne(t *testing.T) {
 	t.Parallel()
-	spec := writeFile(t, "junk.yaml", "hello: world\n")
-	var stdout, stderr bytes.Buffer
-	code := run([]string{"compile", spec}, &stdout, &stderr)
-	assert.Equal(t, 2, code)
-	assert.Contains(t, stderr.String(), "unrecognized spec format")
+	tests := []struct {
+		name, contents, code string
+	}{
+		{"unrecognized format", "hello: world\n", "engine/unrecognized-format"},
+		{"swagger 2.0", "swagger: \"2.0\"\n", "engine/unsupported-format"},
+		{"undecodable source", "openapi: [unterminated\n", "engine/undecodable-source"},
+		{"no compiler for version", "openapi: 4.0.0\ninfo: {title: T, version: \"1\"}\npaths: {}\n",
+			"engine/no-compiler-for-format"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			spec := writeFile(t, "spec.yaml", tt.contents)
+			var stdout, stderr bytes.Buffer
+
+			code := run([]string{"compile", spec}, &stdout, &stderr)
+
+			assert.Equal(t, 1, code, "stderr: %s", stderr.String())
+			assert.Contains(t, stderr.String(), tt.code)
+			assert.Empty(t, stdout.String(), "no IR JSON for a spec that produced no document")
+			assert.NotContains(t, stderr.String(), "usage:",
+				"a bad spec is not a misuse of the CLI, so no usage block")
+		})
+	}
 }
 
 func TestRun_DiagnosticsGateExitCode(t *testing.T) {

cmd/morphic/edgecases_test.go

@@ -258,15 +258,24 @@ func TestRenderDiagnostics_WithAndWithoutSourcePath(t *testing.T) {
 				Message:    "no source file",
 				Provenance: ir.Provenance{Source: 99, Pointer: "type:abc"},
 			},
+			{
+				Severity:   ir.SeverityError,
+				Code:       "engine/unrecognized-format",
+				Message:    "no position at all",
+				Provenance: ir.Provenance{Source: ir.NoSource},
+			},
 		},
 	}
 	var buf bytes.Buffer
 
 	renderDiagnostics(&buf, res)
 
-	out := buf.String()
-	assert.Contains(t, out, "error openapi/bad spec.yaml#/paths/~1x: resolved location")
-	assert.Contains(t, out, "warning ir/dangling type:abc: no source file")
+	// Whole lines, not fragments: the location is what varies between these three
+	// forms, and a containment check on the message alone passes for a line that
+	// renders the location wrongly or fabricates one.
+	assert.Equal(t, "error openapi/bad spec.yaml#/paths/~1x: resolved location\n"+
+		"warning ir/dangling type:abc: no source file\n"+
+		"error engine/unrecognized-format: no position at all\n", buf.String())
 }
 
 func TestWriteDocument_MarshalError(t *testing.T) {

cmd/morphic/validate_test.go

@@ -181,13 +181,17 @@ func TestRun_ValidateUsageErrors(t *testing.T) {
 func TestRun_ValidateEngineFailures(t *testing.T) {
 	t.Parallel()
 
+	// The two rows are different classes and exit differently. A format nothing
+	// recognizes is a problem with the spec, so it is a diagnostic and exits 1; a
+	// file that cannot be read is a problem with the invocation, so it stays a Go
+	// error and exits 2. Nothing about a spec's own contents reaches 2.
 	tests := []struct {
 		name     string
 		spec     string
 		wantCode int
 		wantErr  string
 	}{
-		{"unrecognized spec format", "hello: world\n", 2, "unrecognized spec format"},
+		{"unrecognized spec format", "hello: world\n", 1, "engine/unrecognized-format"},
 		{"unreadable spec file", "", 2, "morphic:"},
 	}
 

compilers/compilers.go

@@ -37,6 +37,12 @@ type Options struct {
 // no package-level mutable state, no writes to stderr; spec problems are
 // returned as ir.Diagnostic values and the error return is reserved for
 // I/O-level and programmer errors.
+//
+// A compiler reports through the returned slice. It may also store the same
+// findings on the Document it returns — that copy is what the persisted IR JSON
+// carries — but nothing obliges it to, and one that fills both must fill them
+// alike. Neither list is guaranteed to hold the other, so a caller holding both
+// unions them, as the engine does, rather than take one for the whole set.
 type Compiler interface {
 	Formats() []SourceFormat
 	Compile(ctx context.Context, sources []Source, opts Options) (*ir.Document, []ir.Diagnostic, error)

engine/diag.go

@@ -0,0 +1,79 @@
+package engine
+
+import (
+	"fmt"
+
+	"github.com/dexpace/morphic/ir"
+)
+
+// Diagnostic codes the engine itself raises, in its own stable namespace beside
+// the compilers' (openapi/*) and the passes' (ir/*), so a CI wrapper can
+// allowlist them by name.
+//
+// Each reports a problem with the source the caller named, not with the call.
+// They exist because the pipeline can reject a spec before any compiler runs,
+// and a rejection is a finding about the spec whichever stage makes it. A Go
+// error out of Run means something other than the spec went wrong: the file
+// could not be read, or a compiler broke its own contract.
+const (
+	// codeUndecodableSource: the bytes parse as neither YAML nor JSON, so nothing
+	// can be read out of them, a format key included.
+	codeUndecodableSource = "engine/undecodable-source"
+	// codeUnrecognizedFormat: the source decoded but declares no key any compiler
+	// in this tree announces itself by.
+	codeUnrecognizedFormat = "engine/unrecognized-format"
+	// codeUnsupportedFormat: the source declares a format Morphic recognizes and
+	// cannot lower yet — Swagger 2.0 today.
+	codeUnsupportedFormat = "engine/unsupported-format"
+	// codeNoCompilerForFormat: the format was read, but no registered compiler
+	// claims it. An OpenAPI version outside the supported range lands here.
+	codeNoCompilerForFormat = "engine/no-compiler-for-format"
+)
+
+// specProblem builds an error-severity diagnostic about a source as a whole.
+// Error is the severity because nothing was lowered: the spec reached the IR in
+// no form at all.
+//
+// The provenance is deliberately positionless. These are raised before anything
+// is lowered, so there is no document whose Sources table an index could
+// address, and ir.NoSource is the IR's value for a finding that names no entry
+// in one. An index invented here would have a renderer point at a file the
+// finding is not about.
+func specProblem(code, format string, args ...any) ir.Diagnostic {
+	return ir.NewDiagnostic(ir.SeverityError, code, fmt.Sprintf(format, args...),
+		ir.Provenance{Source: ir.NoSource})
+}
+
+// mergeDiagnostics returns stored followed by every diagnostic in produced that
+// stored does not already hold. Identity is the whole value — severity, code,
+// message and provenance — the same identity compilers/compile dedupes on.
+//
+// It exists because a compiler hands its findings back on two channels and is
+// obliged to fill only one of them. Assigning either list over the other loses
+// whatever the loser held, in silence and at every severity; merging keeps both
+// and hands a compiler that fills them alike, as the OpenAPI one does, back
+// exactly what it gave.
+//
+// The merged slice is always freshly allocated when there is anything to merge:
+// stored and produced routinely alias one another, and appending into a shared
+// backing array would overwrite entries still to be read.
+func mergeDiagnostics(stored, produced []ir.Diagnostic) []ir.Diagnostic {
+	if len(produced) == 0 {
+		return stored
+	}
+
+	held := make(map[ir.Diagnostic]struct{}, len(stored))
+	for _, d := range stored {
+		held[d] = struct{}{}
+	}
+
+	merged := make([]ir.Diagnostic, len(stored), len(stored)+len(produced))
+	copy(merged, stored)
+	for _, d := range produced {
+		if _, dup := held[d]; dup {
+			continue
+		}
+		merged = append(merged, d)
+	}
+	return merged
+}

engine/engine.go

@@ -19,8 +19,11 @@ type RunOptions struct {
 }
 
 // Result is the outcome of a pipeline run. A nil Document alongside diagnostics
-// is a legal outcome (e.g. an unsupported spec version); the caller decides what
-// is fatal.
+// is a legal outcome and the shape every refusal takes — a source no compiler
+// claims, or one a compiler declined to lower; the caller decides what is fatal.
+//
+// Diagnostics is the whole list for the run. When Document is non-nil it holds
+// the same values, so a caller reading either channel sees every finding.
 type Result struct {
 	Document    *ir.Document           `json:"document,omitempty"`
 	Diagnostics []ir.Diagnostic        `json:"diagnostics,omitempty"`
@@ -59,33 +62,45 @@ func NewWith(fronts ...compilers.Compiler) (*Engine, error) {
 
 // Run executes the pipeline for the spec at specPath: read the file, sniff its
 // format, dispatch to the matching compiler, and — unless disabled — append the
-// validate pass's diagnostics. The Go error return is reserved for I/O and
-// programmer errors; spec problems surface as diagnostics in the Result.
+// validate pass's diagnostics.
+//
+// The Go error return is reserved for I/O and programmer errors: the file could
+// not be read, or a compiler failed in a way its own contract calls an error.
+// Everything wrong with the spec itself comes back as a diagnostic in the
+// Result, a source no compiler can lower included. A caller that treats a Go
+// error as "the pipeline was invoked wrongly" therefore stays correct, which is
+// what lets the CLI keep its usage exit code for actual misuse.
 func (e *Engine) Run(ctx context.Context, specPath string, opts RunOptions) (*Result, error) {
 	data, err := os.ReadFile(specPath)
 	if err != nil {
 		return nil, fmt.Errorf("engine: read spec %q: %w", specPath, err)
 	}
-	format, err := Sniff(data)
-	if err != nil {
-		return nil, fmt.Errorf("engine: sniff %q: %w", specPath, err)
-	}
-	front, ok := e.registry.Lookup(format)
+	format, problem, ok := Sniff(data)
 	if !ok {
-		return nil, fmt.Errorf("engine: no compiler registered for format %s", format)
+		return &Result{Diagnostics: []ir.Diagnostic{problem}}, nil
+	}
+	front, registered := e.registry.Lookup(format)
+	if !registered {
+		return &Result{Format: format, Diagnostics: []ir.Diagnostic{
+			specProblem(codeNoCompilerForFormat, "no compiler registered for format %s", format),
+		}}, nil
 	}
 	doc, diags, err := front.Compile(ctx,
 		[]compilers.Source{{Path: specPath, Data: data}},
 		compilers.Options{FormatOptions: opts.FormatOptions})
 	if err != nil {
 		return nil, fmt.Errorf("engine: parse %q: %w", specPath, err)
 	}
-	if !opts.SkipValidate && doc != nil {
-		// Land the pass diagnostics in the document too, so the persisted IR JSON
-		// (golden snapshots, IR diff, caches, emitters) carries them and does not
-		// silently lose error-level validation findings.
-		doc.Diagnostics = append(doc.Diagnostics, pass.Validate(doc)...)
-		diags = doc.Diagnostics
+	if doc == nil {
+		return &Result{Diagnostics: diags, Format: format}, nil
+	}
+	if !opts.SkipValidate {
+		diags = append(diags, pass.Validate(doc)...)
 	}
-	return &Result{Document: doc, Diagnostics: diags, Format: format}, nil
+	// Both channels end up carrying the whole list: the Result is what a caller
+	// gates on, and the document is what gets persisted (golden snapshots, IR
+	// diff, caches, emitters). Merging rather than picking one is what keeps a
+	// finding its compiler put on only one of them — see mergeDiagnostics.
+	doc.Diagnostics = mergeDiagnostics(doc.Diagnostics, diags)
+	return &Result{Document: doc, Diagnostics: doc.Diagnostics, Format: format}, nil
 }