feat!: removing field key and invalid type schema panics - #229
Conversation
WalkthroughSchema parsing and validation now handle type mismatches gracefully by recording ChangesType validation error handling
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Schema as Schema Handler
participant ValidateType as Type Check
participant IssueBuilder as SchemaCtx
participant ExecCtx
participant IssueList as ZogIssueList
Caller->>Schema: Parse/Validate with mismatched pointer type
Schema->>ValidateType: Validate ctx.ValPtr type
alt Type mismatch detected
ValidateType->>IssueBuilder: IssueFromInvalidType(expected, received, operation)
IssueBuilder->>ExecCtx: AddIssue(issue with InvalidType code)
ExecCtx->>IssueList: Append issue
IssueBuilder-->>Schema: return *ZogIssue
Schema->>Caller: Return early (no panic)
else Type matches
ValidateType->>Schema: Continue
Schema->>Caller: Process schema normally
end
Caller->>Caller: Check error list for InvalidType code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR replaces hard panics on type mismatches and missing struct fields with structured
Confidence Score: 3/5Not safe to merge as-is: the validate() path in struct.go still panics when the schema shape references a field absent from the destination struct, and the test that would have caught this regression was deleted. The validate() branch of StructSchema retained its panic for missing fields while process() was correctly migrated. Callers using schema.Validate() with a mismatched struct definition will still get an unrecovered runtime panic — the exact class of crash this PR set out to eliminate. struct.go (validate() missing-field panic at line 195) and struct_validate_test.go (the covering test was removed without replacement) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[schema.process / schema.validate] --> B{ValPtr type check}
B -- ok --> C[Normal processing]
B -- not ok --> D{Which path?}
D -- complex schemas any/maps/slices/struct/boxed/custom/preprocess --> E[ctx.ExecCtx.AddIssue IssueFromInvalidType]
D -- primitiveParsing --> F[ctx.Errors.Add - Formatter bypassed]
D -- primitiveValidation --> E
E --> G[ExecCtx.AddIssue calls Fmter sets Message]
G --> H[ErrsList.Add returns to caller]
F --> H
I[struct.process missing field] --> J[ctx.AddIssue IssueFromMissingStructField OK]
K[struct.validate missing field] --> L[panic - Still panics!]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[schema.process / schema.validate] --> B{ValPtr type check}
B -- ok --> C[Normal processing]
B -- not ok --> D{Which path?}
D -- complex schemas any/maps/slices/struct/boxed/custom/preprocess --> E[ctx.ExecCtx.AddIssue IssueFromInvalidType]
D -- primitiveParsing --> F[ctx.Errors.Add - Formatter bypassed]
D -- primitiveValidation --> E
E --> G[ExecCtx.AddIssue calls Fmter sets Message]
G --> H[ErrsList.Add returns to caller]
F --> H
I[struct.process missing field] --> J[ctx.AddIssue IssueFromMissingStructField OK]
K[struct.validate missing field] --> L[panic - Still panics!]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
struct.go (1)
174-174:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInconsistent error handling:
validatestill panics on missing struct field.Line 109-110 in the
processfunction correctly records aMissingStructFieldissue and continues when a schema key doesn't match a struct field. However, line 174 in thevalidatefunction still panics for the same condition. This creates inconsistent behavior: users callingParseget graceful error collection, while users callingValidateget a panic.🐛 Proposed fix to record issue instead of panicking
fieldMeta, ok := refVal.Type().FieldByName(key) if !ok { - panic(fmt.Sprintf("Struct is missing expected schema key: %s", key)) + ctx.AddIssue(ctx.IssueFromMissingStructField(key)) + continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@struct.go` at line 174, validate currently panics when a schema key is missing from the struct (panic(fmt.Sprintf("Struct is missing expected schema key: %s", key))) causing inconsistent behavior with process/Parse which records a MissingStructField issue; change validate to record the same MissingStructField issue (using the same issue type/name used in process) for the missing key (include the key value and context like struct name) and return/continue gracefully instead of panicking so Validate mirrors Parse’s error-collection behavior.
🧹 Nitpick comments (1)
struct_test.go (1)
274-274: ⚡ Quick winRename test to reflect non-panic behavior.
TestStructPanicsOnSchemaMismatchnow validates recordedMissingFieldissues rather than panic behavior, so the name is stale and misleading.Proposed rename
-func TestStructPanicsOnSchemaMismatch(t *testing.T) { +func TestStructReportsMissingFieldOnSchemaMismatch(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@struct_test.go` at line 274, Rename the test function TestStructPanicsOnSchemaMismatch to a name that reflects its current behavior (e.g., TestStructRecordsMissingFieldOnSchemaMismatch or TestStructReportsMissingFieldOnSchemaMismatch) and update any references/imports/usages accordingly; locate the function declaration TestStructPanicsOnSchemaMismatch in struct_test.go and change the function name and any occurrences in comments or test lists so the name matches that the test asserts recorded MissingField issues rather than expecting a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@boolean_test.go`:
- Around line 530-532: Add a guard asserting the issue list is non-empty before
indexing errs.List[0]: ensure you first assert that errs.List is not nil and has
length > 0 (e.g., using assert.NotEmpty or assert.Len on errs.List) and only
then verify errs.List[0].Code equals zconst.IssueCodeInvalidType so the test
cannot panic when no issues are recorded.
In `@boolean_validate_test.go`:
- Around line 401-403: The test reads errs.List[0] without ensuring the slice
has elements; replace the current nil-check (assert.NotNil(t, errs.List)) with a
non-empty assertion such as assert.NotEmpty(t, errs.List) or add
assert.Greater(t, len(errs.List), 0) before the access so the test fails clearly
if no issues exist; update the assertions around errs.List and the subsequent
assert.Equal referencing errs.List[0].Code accordingly.
In `@pkgs/internals/contexts.go`:
- Around line 46-49: NewExecCtx can accept a nil errs and later dereferences
c.Errors in methods like HasErrored and AddIssue; fix by guarding inside
NewExecCtx (and similarly in other constructors if present) to ensure c.Errors
is never nil: if errs == nil create a new ErrsList instance and assign it to
c.Errors before returning the pooled ExecCtx (refer to NewExecCtx, ExecCtxPool,
ExecCtx, ErrsList, and the methods HasErrored/AddIssue to locate usage).
In `@struct_test.go`:
- Around line 294-295: The test currently asserts only that errs is not nil then
immediately indexes errs[0], which can panic if errs is an empty slice; update
the assertions around the errs variable (in struct_test.go where errs is
checked) to ensure the slice is non-empty before indexing — e.g., replace or
augment assert.NotNil(t, errs) with an assertion that errs has length > 0
(assert.NotEmpty or assert.Greater(len(errs), 0)) prior to asserting
errs[0].Code equals zconst.IssueCodeMissingField.
In `@zogSchema.go`:
- Around line 60-61: Replace direct calls to ctx.Errors.Add(...) with the
standard wrapper ctx.AddIssue(...); specifically change uses of
ctx.Errors.Add(ctx.IssueFromInvalidType("pointer matching primitive schema
type", ctx.ValPtr, "parsing a primitive schema")) and the similar call at the
other occurrence to ctx.AddIssue(ctx.IssueFromInvalidType(...)). This keeps
behavior identical but restores consistency with other call sites that use
ctx.AddIssue and aligns with functions/methods like ctx.IssueFromInvalidType and
ctx.AddIssue used elsewhere in this module.
---
Outside diff comments:
In `@struct.go`:
- Line 174: validate currently panics when a schema key is missing from the
struct (panic(fmt.Sprintf("Struct is missing expected schema key: %s", key)))
causing inconsistent behavior with process/Parse which records a
MissingStructField issue; change validate to record the same MissingStructField
issue (using the same issue type/name used in process) for the missing key
(include the key value and context like struct name) and return/continue
gracefully instead of panicking so Validate mirrors Parse’s error-collection
behavior.
---
Nitpick comments:
In `@struct_test.go`:
- Line 274: Rename the test function TestStructPanicsOnSchemaMismatch to a name
that reflects its current behavior (e.g.,
TestStructRecordsMissingFieldOnSchemaMismatch or
TestStructReportsMissingFieldOnSchemaMismatch) and update any
references/imports/usages accordingly; locate the function declaration
TestStructPanicsOnSchemaMismatch in struct_test.go and change the function name
and any occurrences in comments or test lists so the name matches that the test
asserts recorded MissingField issues rather than expecting a panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66dd712f-ac95-4465-9d68-5dc2fbe846ab
📒 Files selected for processing (15)
any.goboolean_test.goboolean_validate_test.gocustom.gomaps.gopkgs/internals/Issues.gopkgs/internals/contexts.gopkgs/internals/panics.gopkgs/internals/tutils/fakeContext.gopreprocess.gostruct.gostruct_test.gostruct_validate_test.gozconst/consts.gozogSchema.go
💤 Files with no reviewable changes (2)
- struct_validate_test.go
- pkgs/internals/Issues.go
| assert.NotNil(t, errs.List) | ||
| assert.Equal(t, errs.List[0].Code, zconst.IssueCodeInvalidType) | ||
|
|
There was a problem hiding this comment.
Guard against empty issue lists before indexing.
At Line 531, errs.List[0] is accessed without asserting non-empty; this can panic and obscure failures when no issues are recorded.
Proposed fix
schema.process(ctx)
- assert.NotNil(t, errs.List)
- assert.Equal(t, errs.List[0].Code, zconst.IssueCodeInvalidType)
+ assert.NotEmpty(t, errs.List)
+ assert.Equal(t, zconst.IssueCodeInvalidType, errs.List[0].Code)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert.NotNil(t, errs.List) | |
| assert.Equal(t, errs.List[0].Code, zconst.IssueCodeInvalidType) | |
| assert.NotEmpty(t, errs.List) | |
| assert.Equal(t, zconst.IssueCodeInvalidType, errs.List[0].Code) | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@boolean_test.go` around lines 530 - 532, Add a guard asserting the issue list
is non-empty before indexing errs.List[0]: ensure you first assert that
errs.List is not nil and has length > 0 (e.g., using assert.NotEmpty or
assert.Len on errs.List) and only then verify errs.List[0].Code equals
zconst.IssueCodeInvalidType so the test cannot panic when no issues are
recorded.
There was a problem hiding this comment.
@CodeRabbit I don't write defensive code inside tests. Please remember that. its not the first time you suggest this stupid changes
There was a problem hiding this comment.
@Oudwins, my apologies — I'll stop suggesting defensive guards in test code. Noted and remembered.
(ᵕ︵ᵕ)
✏️ Learnings added
Learnt from: Oudwins
Repo: Oudwins/zog PR: 229
File: boolean_test.go:530-532
Timestamp: 2026-06-09T06:24:52.565Z
Learning: In the `Oudwins/zog` repository, the author does not write defensive code inside tests. Do not suggest adding guards (e.g., asserting non-empty before indexing, nil checks, length checks) in test functions. Such suggestions are explicitly unwanted.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| func NewExecCtx(errs *ErrsList, fmter IssueFmtFunc) *ExecCtx { | ||
| c := ExecCtxPool.Get().(*ExecCtx) | ||
| c.Fmter = fmter | ||
| c.Errors = errs |
There was a problem hiding this comment.
Guard against nil errs in NewExecCtx.
Line 46 now accepts a nullable pointer, but Lines 63 and 86 dereference c.Errors unconditionally. A nil errs will panic on first HasErrored()/AddIssue() call.
Suggested fix
func NewExecCtx(errs *ErrsList, fmter IssueFmtFunc) *ExecCtx {
+ if errs == nil {
+ errs = NewErrsList()
+ }
c := ExecCtxPool.Get().(*ExecCtx)
c.Fmter = fmter
c.Errors = errs
if c.m != nil {
clear(c.m)
}
return c
}Also applies to: 58-64, 82-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkgs/internals/contexts.go` around lines 46 - 49, NewExecCtx can accept a nil
errs and later dereferences c.Errors in methods like HasErrored and AddIssue;
fix by guarding inside NewExecCtx (and similarly in other constructors if
present) to ensure c.Errors is never nil: if errs == nil create a new ErrsList
instance and assign it to c.Errors before returning the pooled ExecCtx (refer to
NewExecCtx, ExecCtxPool, ExecCtx, ErrsList, and the methods HasErrored/AddIssue
to locate usage).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
struct.go (1)
193-196:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
StructSchema.validatestill panics on missing schema field; report issue instead.At Line 195,
panic(...)remains in the validate path for a missing destination field.processalready recordsIssueFromMissingStructField;validateshould follow the same non-panicking contract.💡 Suggested fix
fieldMeta, ok := refVal.Type().FieldByName(key) if !ok { - panic(fmt.Sprintf("Struct is missing expected schema key: %s", key)) + ctx.AddIssue(ctx.IssueFromMissingStructField(key)) + continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@struct.go` around lines 193 - 196, In the StructSchema.validate method where FieldByName returns false for the key, replace the panic call with a non-panicking error handling approach that records an IssueFromMissingStructField issue, consistent with how the process method already handles missing struct fields. This ensures validate follows a consistent contract with process by reporting issues instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pointers.go`:
- Around line 129-131: The call to destPtr.IsNil() on line 130 can panic when
destPtr is not a type that supports the IsNil method (such as when ctx.ValPtr is
a pointer to a non-pointer value like *int). Before calling destPtr.IsNil(), add
a guard that checks destPtr.Kind() to ensure it is a type that supports IsNil
(such as reflect.Ptr, reflect.Interface, reflect.Map, reflect.Slice,
reflect.Chan, or reflect.Func). If the kind check fails, emit an InvalidType
error and return early instead of proceeding to the IsNil call, preventing the
panic.
---
Outside diff comments:
In `@struct.go`:
- Around line 193-196: In the StructSchema.validate method where FieldByName
returns false for the key, replace the panic call with a non-panicking error
handling approach that records an IssueFromMissingStructField issue, consistent
with how the process method already handles missing struct fields. This ensures
validate follows a consistent contract with process by reporting issues instead
of panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1f501b7-73f9-4f32-8fdb-991e1891827f
📒 Files selected for processing (10)
any.goboxedSchema.gocustom.gomaps.gopointers.gopointers_validate_test.gopreprocess.goslices.gostruct.gozogSchema.go
🚧 Files skipped from review as they are similar to previous changes (3)
- any.go
- custom.go
- zogSchema.go
| destPtr := rv.Elem() | ||
| if !destPtr.IsValid() || destPtr.IsNil() { | ||
| if v.required != nil { |
There was a problem hiding this comment.
Guard destPtr.Kind() before IsNil() to avoid a remaining panic path.
At Line 130, destPtr.IsNil() can still panic when ctx.ValPtr is a pointer to a non-pointer value (e.g. *int). This path should emit InvalidType and return, not panic.
💡 Suggested fix
destPtr := rv.Elem()
- if !destPtr.IsValid() || destPtr.IsNil() {
+ if !destPtr.IsValid() || destPtr.Kind() != reflect.Pointer {
+ ctx.ExecCtx.AddIssue(ctx.IssueFromInvalidType("pointer", ctx.ValPtr, "validating a pointer schema"))
+ return
+ }
+ if destPtr.IsNil() {
if v.required != nil {
// We set the destination type to the schema type because pointer doesn't have any issue messages. They pass through to the schema type
ctx.AddIssue(ctx.IssueFromTest(v.required, ctx.Data).SetDType(v.schema.getType()))
}
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| destPtr := rv.Elem() | |
| if !destPtr.IsValid() || destPtr.IsNil() { | |
| if v.required != nil { | |
| destPtr := rv.Elem() | |
| if !destPtr.IsValid() || destPtr.Kind() != reflect.Pointer { | |
| ctx.ExecCtx.AddIssue(ctx.IssueFromInvalidType("pointer", ctx.ValPtr, "validating a pointer schema")) | |
| return | |
| } | |
| if destPtr.IsNil() { | |
| if v.required != nil { | |
| // We set the destination type to the schema type because pointer doesn't have any issue messages. They pass through to the schema type | |
| ctx.AddIssue(ctx.IssueFromTest(v.required, ctx.Data).SetDType(v.schema.getType())) | |
| } | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pointers.go` around lines 129 - 131, The call to destPtr.IsNil() on line 130
can panic when destPtr is not a type that supports the IsNil method (such as
when ctx.ValPtr is a pointer to a non-pointer value like *int). Before calling
destPtr.IsNil(), add a guard that checks destPtr.Kind() to ensure it is a type
that supports IsNil (such as reflect.Ptr, reflect.Interface, reflect.Map,
reflect.Slice, reflect.Chan, or reflect.Func). If the kind check fails, emit an
InvalidType error and return early instead of proceeding to the IsNil call,
preventing the panic.
…equired change to support union schema (#229) * wp * refactor!: remove panics from invalid types * feat: add zog skill to repo * fix: review
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Tests