Repository navigation
feat: add strict gRPC contracts - #1
Conversation
There was a problem hiding this comment.
Pull request overview
Adds protobuf-backed “strict” contracts for audit modules/steps/service methods and wires typed providers + a runtime contract registry so the plugin can be validated/consumed via gRPC/typed adapters.
Changes:
- Introduce
plugin.contracts.json+ protobuf definitions for strict module/step/service contracts. - Add typed module/step creation + contract registry exposure in the plugin implementation.
- Update build/release/CI plumbing (ldflags versioning, goreleaser packaging, GOWORK=off CI) and bump dependencies/engine minimum.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin.json | Bumps version/minEngineVersion; adds download artifacts + declares service methods capability. |
| plugin.contracts.json | Declares strict contract manifest for modules/steps/service methods. |
| internal/typed.go | Implements typed step handlers and typed service method invocation + proto↔map adapters. |
| internal/plugin_contracts_test.go | Adds tests validating strict contracts, registry, and typed behavior. |
| internal/plugin.go | Adds typed providers and contract registry; makes type lists defensive; returns errors for unknown types. |
| internal/contracts/audit.proto | Defines protobuf message contracts for configs/inputs/outputs and service request/response types. |
| internal/contracts/audit.pb.go | Generated Go code for the protobuf contracts. |
| go.mod | Bumps workflow engine dependency and adds/upgrades related deps. |
| go.sum | Updates checksums for upgraded dependencies. |
| Makefile | Adds build/test/install targets with ldflags version wiring and contracts install packaging. |
| .goreleaser.yaml | Updates plugin.json sed to rewrite download URLs; includes plugin.contracts.json in archives. |
| .github/workflows/ci.yml | Forces GOWORK=off for build/test/vet in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| events = append(events, auditEvent) | ||
| } | ||
| raw, _ := json.Marshal(events) |
There was a problem hiding this comment.
exportInputToMap ignores the json.Marshal error when encoding events (raw, _ := json.Marshal(events)). If marshaling fails, this currently returns an empty/partial map without surfacing the failure, which can make typed export behave incorrectly and hide data-loss bugs. Return the marshal error to the caller instead of discarding it.
| raw, _ := json.Marshal(events) | |
| raw, err := json.Marshal(events) | |
| if err != nil { | |
| return nil, err | |
| } |
There was a problem hiding this comment.
Addressed in e1d51b8 by returning the json.Marshal error from exportInputToMap instead of discarding it.
| func protoMessageToMap(msg proto.Message) map[string]any { | ||
| if msg == nil { | ||
| return nil | ||
| } | ||
| raw, err := (protojson.MarshalOptions{UseProtoNames: true}).Marshal(msg) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| values := map[string]any{} | ||
| if err := json.Unmarshal(raw, &values); err != nil { | ||
| return nil | ||
| } | ||
| return values | ||
| } |
There was a problem hiding this comment.
protoMessageToMap is currently unused in this package, and it also swallows marshal/unmarshal errors by returning nil. Consider removing it if it’s dead code, or changing it to return (map[string]any, error) so callers can distinguish a real empty map from a conversion failure.
There was a problem hiding this comment.
Addressed in e1d51b8 by removing the unused protoMessageToMap helper entirely.
| func parseOptionalQueryTimestamp(name, raw string) (*time.Time, error) { | ||
| if raw == "" { | ||
| return nil, nil | ||
| } | ||
| parsed, err := time.Parse(time.RFC3339Nano, raw) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%s timestamp %q: %w", name, raw, err) | ||
| } | ||
| return &parsed, nil |
There was a problem hiding this comment.
parseOptionalQueryTimestamp validates query timestamps using time.RFC3339Nano, but the runtime query parsing in AuditQueryStep / CollectorModule uses time.RFC3339. This can accept timestamps with fractional seconds during typed validation that will later be ignored by the runtime parser, leading to inconsistent query behavior. Align both sides to the same layout (either switch validation to RFC3339, or update the runtime parsers to RFC3339Nano) and add/adjust a test to cover fractional-second timestamps.
There was a problem hiding this comment.
Addressed in e1d51b8 by aligning runtime query parsing to RFC3339Nano and adding a typed fractional-second bounds regression test.
Summary
Verification
Review