Refactor schema compilation to use a single shared helper#42614
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors JSON Schema compilation to eliminate duplicate compiler setup logic by centralizing it in pkg/parser and reusing it from pkg/workflow, while adding a focused unit test for the shared helper.
Changes:
- Exported
parser.CompileSchemaas the shared JSON Schema compilation helper (retaining a package-local wrapper for existing internal call sites). - Updated
pkg/workflow’s schema helper to delegate compilation toparser.CompileSchema. - Added a unit test covering compile + validate behavior for the shared helper.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/schema_utils.go | Replaces the local schema compiler implementation with a delegation to parser.CompileSchema. |
| pkg/parser/schema_compiler.go | Exports CompileSchema and keeps the original compileSchema wrapper for internal callers. |
| pkg/parser/schema_compiler_test.go | Adds a unit test to validate successful compilation and basic validation behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| // compileSchema preserves existing package-local call sites. | ||
| func compileSchema(schemaJSON, schemaURL string) (*jsonschema.Schema, error) { | ||
| return CompileSchema(schemaJSON, schemaURL) | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42614 does not have the 'implementation' label and has only 38 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Score breakdown:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refactor schema compilation to use a single shared helper
The refactoring is correct and well-scoped — no import cycles, no logic changes, and the workflow package correctly delegates to the shared parser.CompileSchema. Three non-blocking observations:
Summary of comments
-
Error message regression (
schema_compiler.goline 141): The oldworkflow.compileSchemaincludedschemaURLin all three error messages, but the newCompileSchemadrops it (URL is only logged). This subtly degrades debuggability when a schema fails to compile at runtime. -
Unnecessary private wrapper (
schema_compiler.goline 149): The new privatecompileSchemais a one-liner that exists only to avoid updating 4 internal call sites. Updating those call sites to useCompileSchemadirectly would remove this layer of indirection. -
Missing error-path test (
schema_compiler_test.goline 29): The test covers the happy path and type validation but not the malformed-JSON error path throughCompileSchema.
None of these block merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 36.6 AIC · ⌖ 9.92 AIC · ⊞ 4.8K
Comments that could not be inline-anchored
pkg/parser/schema_compiler.go:141
The old workflow.compileSchema included schemaURL in all three error messages (e.g. "failed to parse schema JSON for %s: %w", schemaURL, err), but CompileSchema drops it from returned errors — the URL is only logged. When multiple schemas are compiled and one fails at runtime, callers lose the URL context in the error chain.
Consider including schemaURL in each error string for easier diagnosis:
return nil, fmt.Errorf("failed to parse schema JSON for %s: %w", schemaURL, err)
re…
</details>
<details><summary>pkg/parser/schema_compiler.go:149</summary>
The private `compileSchema` wrapper is now a one-liner with no extra logic — it exists only to avoid updating the 4 internal call sites in this file. The cleaner long-term approach is to replace those call sites with `CompileSchema` directly and remove this wrapper, reducing indirection:
```go
// Before:
compileSchema(mainWorkflowSchema, "(contoso.com/redacted)
// After:
CompileSchema(mainWorkflowSchema, "(contoso.com/redacted)This is a non-blocking suggestion — the wrapper is not wrong,…
pkg/parser/schema_compiler_test.go:29
The happy path and type-mismatch validation are well covered. Consider adding an error-path case for malformed JSON input to verify the first Unmarshal error is correctly propagated:
_, err = CompileSchema(`{not valid json}`, "(example.com/redacted)
if err == nil {
t.Fatal("CompileSchema(bad JSON) returned nil error, want parse failure")
}This would ensure the error wrapping in CompileSchema works end-to-end.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — leaving as COMMENT since no blocking issues were found. Three targeted improvements worth addressing.
📋 Key Themes & Highlights
Key Themes
- Stale doc comment (
pkg/workflow/schema_utils.go): ThecompileSchemawrapper still advertisesNewCompiler → AddResource → Compileboilerplate that no longer lives here — misleads readers navigating the codebase. - Unnecessary private shim (
pkg/parser/schema_compiler.go): The privatecompileSchemawrapper is a trivial one-liner kept only to avoid touching internal call sites. Updating those call sites directly would make the package cleaner. - Missing error-path test (
pkg/parser/schema_compiler_test.go): The new test validates happy-path compile and both valid/invalid data inputs, but does not specify what happens whenschemaJSONis itself not valid JSON — the error contract is untested.
Positive Highlights
- ✅ Clean, well-scoped refactor — no behaviour changes, exactly as described
- ✅ New cross-package dependency direction (
workflow→parser) is natural and correct - ✅ Test uses
t.Parallel()and covers both the valid and invalid validation branches - ✅ Good use of
//go:build !integrationto keep the test in the fast suite
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 38.8 AIC · ⌖ 7.82 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
|
|
||
| // compileSchema preserves existing package-local call sites. | ||
| func compileSchema(schemaJSON, schemaURL string) (*jsonschema.Schema, error) { |
There was a problem hiding this comment.
[/codebase-design] The private compileSchema wrapper is a trivial one-liner that adds indirection without benefit — internal callers could call CompileSchema directly.
💡 Suggested alternative
Instead of keeping this private shim, update the existing internal call sites in pkg/parser to call CompileSchema directly and remove the wrapper:
// Before (internal callers)
schema, err := compileSchema(json, url)
// After
schema, err := CompileSchema(json, url)The shim exists "to preserve existing call sites" per the PR description, but the cost is a dead-end indirection that makes the package harder to navigate. If the goal is to keep the diff small, that is reasonable — but a follow-up to remove it would clean the API surface.
@copilot please address this.
| @@ -12,20 +11,5 @@ import ( | |||
| // It is a shared helper used by all schema-compilation sites in this package to avoid | |||
| // repeating the NewCompiler → AddResource → Compile boilerplate. | |||
There was a problem hiding this comment.
[/codebase-design] The doc comment still references the concrete boilerplate (NewCompiler → AddResource → Compile) that no longer lives here — it now lives in parser.CompileSchema. A reader of this file will look for that boilerplate and not find it, which creates confusion.
💡 Suggested wording
// compileSchema is a package-local alias that delegates to parser.CompileSchema,
// the shared JSON Schema compilation helper for the entire project.
func compileSchema(schemaJSON, schemaURL string) (*jsonschema.Schema, error) {Or simply drop the boilerplate description entirely since the body is now self-explanatory.
@copilot please address this.
|
|
||
| import "testing" | ||
|
|
||
| func TestCompileSchema(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] TestCompileSchema does not exercise the error path when schemaJSON is not valid JSON. The error contract of CompileSchema is unspecified by the test suite — a future refactor could silently change error behaviour.
💡 Suggested addition using subtests
Consider restructuring as table-driven subtests so each scenario is independently reported, and add an error-path case:
func TestCompileSchema(t *testing.T) {
t.Parallel()
t.Run("valid schema", func(t *testing.T) {
t.Parallel()
schema, err := CompileSchema(`{"type":"object","properties":{"name":{"type":"string"}},"required":["name"],"additionalProperties":false}`, "(example.com/redacted)
if err != nil {
t.Fatalf("CompileSchema returned error: %v", err)
}
if err := schema.Validate(map[string]any{"name": "gh-aw"}); err != nil {
t.Fatalf("Validate(valid) returned error: %v", err)
}
if err := schema.Validate(map[string]any{"name": 1}); err == nil {
t.Fatal("Validate(invalid) returned nil error, want validation failure")
}
})
t.Run("invalid JSON input", func(t *testing.T) {
t.Parallel()
_, err := CompileSchema("not-valid-json", "(example.com/redacted)
if err == nil {
t.Fatal("CompileSchema returned nil error for invalid JSON, want error")
}
})
}@copilot please address this.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 57.7 AIC · ⌖ 7.14 AIC · ⊞ 1.6K
Comment /review to run again
| } | ||
|
|
||
| // compileSchema preserves existing package-local call sites. | ||
| func compileSchema(schemaJSON, schemaURL string) (*jsonschema.Schema, error) { |
There was a problem hiding this comment.
Unnecessary shim creates a two-name problem in the same package — the 5 internal callers (lines 68, 76, 84, 92, 305) could be trivially updated to CompileSchema directly, eliminating this wrapper entirely.
💡 Why this matters
With both compileSchema and CompileSchema in the package, any new internal code must decide which to call — and the unexported name silently 'wins' for internal use since it is the familiar name. This creates ongoing confusion about intent: is there a reason to call the wrapper vs. the real function?
The fix is a 5-line search-replace across this file — replacing compileSchema( with CompileSchema( at the 5 call sites — which is lower-risk and leaves no residual ambiguity. The wrapper can then be deleted entirely.
| if err := schema.Validate(map[string]any{"name": 1}); err == nil { | ||
| t.Fatal("Validate(invalid) returned nil error, want validation failure") | ||
| } | ||
| } |
There was a problem hiding this comment.
Test has zero coverage of CompileSchema's error paths — only the happy path and schema-validation failure are exercised; the JSON unmarshal, AddResource, and Compile error branches are completely untested.
💡 Suggested addition
func TestCompileSchema_InvalidJSON(t *testing.T) {
t.Parallel()
_, err := CompileSchema(`{not valid json}`, "(example.com/redacted)
if err == nil {
t.Fatal("CompileSchema with malformed JSON should return an error")
}
}
func TestCompileSchema_InvalidSchema(t *testing.T) {
t.Parallel()
// references an undefined — compile step should fail
_, err := CompileSchema(`{"": "#/definitions/missing"}`, "(example.com/redacted)
if err == nil {
t.Fatal("CompileSchema with unresolvable $ref should return an error")
}
}The PR description specifically claims "validate compile + validate behavior directly"; testing only the sunny-day path does not validate the compile error branch, which is exactly the code that just moved between packages and is most likely to have a subtle behavioural gap.
This change removes the only real duplicate found by the semantic function clustering sweep: two package-local
compileSchemaimplementations that performed the same JSON Schema compiler setup. The workflow package now reuses the parser package’s shared implementation instead of maintaining a second copy.What changed
parser.CompileSchemaas the shared JSON Schema compilation helper.pkg/workflow/schema_utils.goto delegate toparser.CompileSchema.Why this is scoped narrowly
pkg/workflowcontinue to use the same local helper shape; only the implementation source changed.Coverage