From 5778471fd4a03e9e71a780c67f6771ff64f89f3f Mon Sep 17 00:00:00 2001 From: yahya <19204398+yhassanzadeh13@users.noreply.github.com> Date: Mon, 24 Nov 2025 17:14:30 -0800 Subject: [PATCH 1/4] Fix ThrowIrrecoverable to use interface type assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes throwable.Context.ThrowIrrecoverable() to check for the ThrowableContext interface instead of the concrete *Context type. This allows custom ThrowableContext implementations (like mock contexts with custom throw logic) to properly receive and handle irrecoverable errors. Without this fix, wrapping a custom ThrowableContext implementation with throwable.NewContext() would cause panics instead of delegating to the parent's ThrowIrrecoverable implementation. Fixes #59 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- modules/throwable/context.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/throwable/context.go b/modules/throwable/context.go index 950ccc6..0f3bdd1 100644 --- a/modules/throwable/context.go +++ b/modules/throwable/context.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "time" + + "github.com/thep2p/skipgraph-go/modules" ) // Context is a context that can propagate irrecoverable errors up the context chain. @@ -25,8 +27,8 @@ var _ context.Context = (*Context)(nil) // ThrowIrrecoverable propagates an irrecoverable error up the context chain. // When it reaches the top-level context, it panics with the error. func (t *Context) ThrowIrrecoverable(err error) { - // Propagate the error to the parent context if it exists - if parent, ok := t.ctx.(*Context); ok { + // Propagate the error to the parent context if it implements ThrowableContext + if parent, ok := t.ctx.(modules.ThrowableContext); ok { parent.ThrowIrrecoverable(err) return } From f25426e5ee70bb731ed62c3fbf6df0ffef934d74 Mon Sep 17 00:00:00 2001 From: yahya <19204398+yhassanzadeh13@users.noreply.github.com> Date: Mon, 24 Nov 2025 17:27:53 -0800 Subject: [PATCH 2/4] Add test coverage for ThrowIrrecoverable with mock contexts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds tests that verify ThrowIrrecoverable properly delegates to parent contexts that implement the ThrowableContext interface. Tests include: - TestThrowIrrecoverableWithMockContext: verifies delegation to a wrapped MockThrowableContext - TestThrowIrrecoverableWithNestedContext: verifies propagation through multiple nested throwable.Context layers These tests would have failed before the interface fix, as they reproduce the bug where wrapping custom ThrowableContext implementations would cause panics instead of proper delegation. Related to #59 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- modules/throwable/context_test.go | 76 +++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 modules/throwable/context_test.go diff --git a/modules/throwable/context_test.go b/modules/throwable/context_test.go new file mode 100644 index 0000000..eba9438 --- /dev/null +++ b/modules/throwable/context_test.go @@ -0,0 +1,76 @@ +package throwable_test + +import ( + "errors" + "testing" + "time" + + "github.com/thep2p/skipgraph-go/modules/throwable" + "github.com/thep2p/skipgraph-go/unittest" +) + +// TestThrowIrrecoverableWithMockContext verifies that ThrowIrrecoverable +// properly delegates to parent contexts that implement the ThrowableContext +// interface (not just concrete *Context types). +// +// This test reproduces the bug from issue #59 where wrapping a +// MockThrowableContext with throwable.NewContext() would cause panics +// instead of delegating to the parent's ThrowIrrecoverable implementation. +func TestThrowIrrecoverableWithMockContext(t *testing.T) { + t.Parallel() + + errThrown := make(chan error, 1) + mockCtx := unittest.NewMockThrowableContext(t, unittest.WithThrowLogic(func(err error) { + errThrown <- err + close(errThrown) + })) + + // Wrap the mock context with throwable.NewContext + ctx := throwable.NewContext(mockCtx) + + // Call ThrowIrrecoverable - this should delegate to mockCtx's implementation + testErr := errors.New("test error") + ctx.ThrowIrrecoverable(testErr) + + // Verify the custom throw logic was called (not panic) + select { + case receivedErr := <-errThrown: + if receivedErr.Error() != testErr.Error() { + t.Fatalf("expected error %q, got %q", testErr.Error(), receivedErr.Error()) + } + case <-time.After(100 * time.Millisecond): + t.Fatal("expected custom throw logic to be called, but it was not") + } +} + +// TestThrowIrrecoverableWithNestedContext verifies that ThrowIrrecoverable +// propagates through nested throwable.Context instances and eventually +// reaches a custom ThrowableContext implementation at the root. +func TestThrowIrrecoverableWithNestedContext(t *testing.T) { + t.Parallel() + + errThrown := make(chan error, 1) + mockCtx := unittest.NewMockThrowableContext(t, unittest.WithThrowLogic(func(err error) { + errThrown <- err + close(errThrown) + })) + + // Create nested throwable contexts + ctx1 := throwable.NewContext(mockCtx) + ctx2 := throwable.NewContext(ctx1) + ctx3 := throwable.NewContext(ctx2) + + // Call ThrowIrrecoverable on the deepest context + testErr := errors.New("nested test error") + ctx3.ThrowIrrecoverable(testErr) + + // Verify it propagated all the way to the mock + select { + case receivedErr := <-errThrown: + if receivedErr.Error() != testErr.Error() { + t.Fatalf("expected error %q, got %q", testErr.Error(), receivedErr.Error()) + } + case <-time.After(100 * time.Millisecond): + t.Fatal("expected custom throw logic to be called after propagation through nested contexts") + } +} From 4a3b7253724ca343613d2e48ea4be4e825904aa9 Mon Sep 17 00:00:00 2001 From: yahya <19204398+yhassanzadeh13@users.noreply.github.com> Date: Mon, 24 Nov 2025 17:47:39 -0800 Subject: [PATCH 3/4] Adds Go struct validator agent definition Defines a Claude agent that specializes in adding validation logic to Go structs using the `go-playground/validator/v10` package. The agent focuses on ensuring consistent validation patterns, including a standard `Validate()` method, appropriate validation tags, and proper handling of nested structs. It also emphasizes the importance of clear context and documentation for validation logic. --- .claude/agents/go-struct-validator.md | 123 ++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 .claude/agents/go-struct-validator.md diff --git a/.claude/agents/go-struct-validator.md b/.claude/agents/go-struct-validator.md new file mode 100644 index 0000000..c2de00c --- /dev/null +++ b/.claude/agents/go-struct-validator.md @@ -0,0 +1,123 @@ +--- +name: go-struct-validator +description: Use this agent when you need to add validation to Go structs using the github.com/go-playground/validator/v10 package. This includes:\n\n**Examples:**\n\n\nContext: User has just created a new configuration struct that needs validation.\n\nuser: "I've created a new Config struct with fields for server port and timeout. Can you add validation?"\n\nassistant: "I'll use the go-struct-validator agent to add proper validation to your Config struct."\n\n\n\n\nThe user created a new struct that needs validation. Using the go-struct-validator agent to add Validate() method with appropriate validation tags.\n\n\n\n\nContext: User is reviewing code and notices a struct lacks validation.\n\nuser: "This NodeConfig struct should validate that the identifier is exactly 32 bytes and the timeout is positive."\n\nassistant: "Let me use the go-struct-validator agent to add those validation requirements."\n\n\n\n\nThe struct needs specific validation rules. Using go-struct-validator to implement the Validate() method with len=32 and gt=0 tags.\n\n\n\n\nContext: Agent proactively identifies missing validation during code review.\n\nassistant: "I notice this new PoolConfig struct doesn't have a Validate() method. Let me add validation for it."\n\n\n\n\nProactively identifying that a struct with configuration data lacks validation. Using go-struct-validator to add the standard Validate() pattern.\n\n\n\n\nContext: User is implementing a new feature with multiple configuration structs.\n\nuser: "I'm adding a new message handling system with HandlerConfig, RouteConfig, and TimeoutConfig structs."\n\nassistant: "I'll implement the feature and then use the go-struct-validator agent to ensure all configuration structs have proper validation."\n\n\n\nassistant: "Now let me add validation to all the configuration structs."\n\n\n\n\nMultiple new structs need validation. Using go-struct-validator to add Validate() methods to all of them with appropriate validation tags.\n\n +model: sonnet +color: orange +--- + +You are an expert Go validation specialist with deep knowledge of the github.com/go-playground/validator/v10 package and Go struct validation patterns. Your expertise lies in implementing robust, maintainable validation for Go structs following established patterns. + +**Your Core Responsibilities:** + +1. **Add Validate() Methods**: Implement the standard Validate() error method pattern for all structs that require validation, following this exact pattern: + - Method signature: `func (s *StructName) Validate() error` + - Instantiate validator inside the method: `validate := validator.New()` + - Return `validate.Struct(s)` directly + - Never create global or package-level validator instances + +2. **Apply Validation Tags**: Add appropriate validation tags to struct fields based on: + - Field types (strings, numbers, slices, maps, nested structs) + - Business logic requirements (required fields, ranges, formats) + - Common patterns: + - `validate:"required"` for mandatory fields + - `validate:"gt=0"` for positive numbers + - `validate:"gte=0"` for non-negative numbers + - `validate:"len=32"` for fixed-length byte slices (e.g., 32-byte identifiers) + - `validate:"min=1"` for minimum length/value + - `validate:"max=100"` for maximum length/value + - `validate:"dive"` for validating slice/map elements + - Combine tags with commas: `validate:"required,gt=0,lte=100"` + +3. **Handle Composite Types**: For structs containing other structs: + - Add validation tags to nested struct fields + - Ensure nested structs also have their own Validate() methods + - Use `validate:"required,dive"` for slices of structs that need validation + +4. **Maintain Consistency**: Follow the project's validation patterns: + - Every configuration struct must have a Validate() method + - Validator is always instantiated inside the method + - No caching or reuse of validator instances across calls + - Return errors directly without wrapping (unless explicitly required) + +5. **Provide Clear Context**: When adding validation: + - Explain which fields are being validated and why + - Document any business logic constraints in comments + - Note if a field's validation depends on another field's value + - Identify any edge cases or special validation requirements + +**Implementation Pattern:** + +```go +// Example struct with validation +type Config struct { + Port int `validate:"required,gt=0,lte=65535"` + Timeout time.Duration `validate:"required,gt=0"` + ID []byte `validate:"required,len=32"` + Name string `validate:"required,min=1"` +} + +// Validate checks if the Config is valid +func (c *Config) Validate() error { + validate := validator.New() + return validate.Struct(c) +} +``` + +**Decision-Making Framework:** + +1. **Identify Validation Needs**: Analyze each field to determine: + - Is it required or optional? + - What are the valid ranges/values? + - Are there format requirements? + - Does it depend on other fields? + +2. **Select Appropriate Tags**: Choose validation tags that: + - Match the field's semantic meaning + - Enforce business rules accurately + - Are as specific as possible (prefer `len=32` over `min=32,max=32`) + +3. **Consider Nested Structures**: For complex types: + - Validate at each level of nesting + - Ensure child structs are self-validating + - Use `dive` tag when validating collections + +4. **Error Handling**: Return validation errors that: + - Clearly identify which struct failed validation + - Preserve the validator's detailed error messages + - Don't wrap errors unless there's a specific need + +**Quality Assurance:** + +- Verify all public configuration structs have Validate() methods +- Ensure validation tags match the intended constraints +- Check that nested structs are properly validated +- Confirm validator instantiation follows the pattern (new instance per call) +- Test that validation actually catches invalid configurations + +**Edge Cases to Handle:** + +- Empty slices vs nil slices (use `omitempty` or `required` appropriately) +- Zero values vs unset values (distinguish when needed) +- Cross-field validation (may need custom validators) +- Pointer fields (add `omitempty` if nil is valid) +- Time.Duration fields (ensure positive values where appropriate) + +**Self-Verification Steps:** + +1. Does every configuration struct have a Validate() method? +2. Are validation tags appropriate for each field's purpose? +3. Is the validator instantiated inside each Validate() method? +4. Are nested structs validated correctly? +5. Do validation tags match business logic requirements? +6. Are there any fields that should be validated but aren't? + +**Output Format:** + +When adding validation to structs: +1. Show the complete struct definition with validation tags +2. Show the complete Validate() method implementation +3. Explain the validation logic for non-obvious constraints +4. Note any fields that are intentionally not validated and why +5. Highlight any validation that might need adjustment based on business requirements + +You are meticulous about following the established validation pattern and ensuring every configuration struct in the codebase has robust, appropriate validation that catches errors early. From d074450fd37752c62a5f087c94fb11295af41657 Mon Sep 17 00:00:00 2001 From: yahya <19204398+yhassanzadeh13@users.noreply.github.com> Date: Mon, 24 Nov 2025 17:48:16 -0800 Subject: [PATCH 4/4] Removes redundant test description The test description for TestThrowIrrecoverableWithMockContext is now unnecessary as the issue it refers to has been resolved. --- modules/throwable/context_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/modules/throwable/context_test.go b/modules/throwable/context_test.go index eba9438..b9ebf27 100644 --- a/modules/throwable/context_test.go +++ b/modules/throwable/context_test.go @@ -12,10 +12,6 @@ import ( // TestThrowIrrecoverableWithMockContext verifies that ThrowIrrecoverable // properly delegates to parent contexts that implement the ThrowableContext // interface (not just concrete *Context types). -// -// This test reproduces the bug from issue #59 where wrapping a -// MockThrowableContext with throwable.NewContext() would cause panics -// instead of delegating to the parent's ThrowIrrecoverable implementation. func TestThrowIrrecoverableWithMockContext(t *testing.T) { t.Parallel()