WS02: Protocol v2 — .proto file and generated Go bindings by brokenbot · Pull Request #139 · brokenbots/criteria

brokenbot · 2026-05-16T21:05:56Z

Summary

WS02 delivers the adapter protocol v2 surface and generated Go bindings, plus protocol helpers/tests needed for WS03/SDK adoption, while preserving v1 behavior.

Adds proto/criteria/v2/options.proto with criteria.sensitive field option.
Adds proto/criteria/v2/adapter.proto with the approved v2 RPC/message surface and reserved ranges.
Adds generated Go bindings (adapter.pb.go, adapter_grpc.pb.go, options.pb.go) and v2 helper/test coverage (chunking, heartbeat, canonical audit JSON).
Includes additive CI/proto-generation wiring for proto drift checks.

Test evidence (from workstream + reviewer notes)

unset CRITERIA_LOCAL_APPROVAL && make ci — passed
go test -race -count=1 ./proto/criteria/v2/... ./internal/adapter/audit/... — passed
go vet ./proto/criteria/v2/... ./internal/adapter/audit/... — passed
make lint-go — passed
make lint-imports — passed

Reviewer notes

Workstream reviewer entries include approvals on 2026-05-17-05 and 2026-05-17-06, with the latest note indicating the final commit is documentation-only and that WS02 remains within scope with prior validation intact.

handcaught

Thanks for the thorough work — generated bindings, helpers, reservations, fuzz target, and tests all hang together cleanly, and the workstream narrative captures the prior remediation history well. Before this can land, a few concrete protocol and code-quality items need to be addressed; they are all called out inline so each thread can be resolved on its own.

The main contract concern is the chunking story for the three fields the workstream calls out as chunkable (AdapterEvent.payload, LogEvent.line, ExecuteResult.outputs): two are non-byte typed fields, the third is a proto3 string, and the SplitChunks helper produces [][]byte. So the on-wire Chunk metadata is correct, but no caller can actually consume the helper to populate any of the three documented fields safely. That needs to be reconciled before WS03/SDK adopt this surface.

The remaining items are smaller (a uint32 truncation in NeedsChunking, two test-readability nits, and a test that asserts envelope round-trips without ever attaching a payload to a wire field). Once these are addressed I expect this to be approvable on the next pass.

handcaught · 2026-05-16T21:25:13Z

+  string                    session_id  = 1;
+  string                    step_name   = 2;
+  string                    stream_name = 3;
+  string                    line        = 4;


line is string here, but proto3 strings must be valid UTF-8 and protobuf-go's decoder enforces that on Unmarshal. The chunk doc on line 203 says chunks are reassembled by concatenating line across messages — but if SplitChunks splits a payload mid-codepoint (e.g. byte 2 of a 3-byte rune), the resulting partial-string chunk is invalid UTF-8 and the receiver will reject it. v1 sidestepped this by using bytes chunk on LogEvent (see proto/criteria/v1/adapter_plugin.proto:65); v2 has regressed.

Please change line to bytes (the natural type for a payload split by byte length), or document and enforce that chunkers must only split on UTF-8 codepoint boundaries and add a test that proves multi-byte content round-trips when split at a worst-case boundary (e.g. a 4-byte emoji that lands across two chunks).

handcaught · 2026-05-16T21:25:13Z

+// chunk is non-nil when payload is a partial segment of a larger JSON payload.
+message AdapterEvent {
+  string                    event_kind  = 1;
+  google.protobuf.Struct    payload     = 2;


AdapterEvent.payload is google.protobuf.Struct, but the workstream lists this field as one of three that must support chunking (WS02 line 141). A Struct is structured data — there is no defined way to serialize a partial Struct such that seq=0..total-1 chunks reassemble back into the original Struct on the receiver. SplitChunks([]byte, ...) can't produce a Struct-shaped partial, and the message has no bytes-side-channel field. As shipped the wire envelope can't actually carry a chunked payload.

Please pick one of: (a) change payload to bytes and document the agreed encoding (e.g. canonical JSON); (b) add a sibling bytes payload_chunk field used only when chunk != nil and document that receivers ignore payload until all chunks arrive; or (c) narrow the workstream so AdapterEvent.payload is not in the chunkable set, and remove the chunk field here. Whichever path is taken, add a round-trip test that actually splits, marshals, unmarshals, and reassembles a typed value.

handcaught · 2026-05-16T21:25:13Z

+  string              outcome = 1;
+  map<string, string> outputs = 2;
+  // chunk is non-nil when outputs is a partial segment; reassemble before use.
+  Chunk               chunk   = 3;


Same shape problem as AdapterEvent.payload: outputs is a map<string, string>, but it is named as chunkable on WS02 line 141 and SplitChunks returns [][]byte. The plausible semantics (partition map entries across total messages, union on receive) is workable but is neither defined in the proto comment nor exercised by a test.

Please either pick a concrete chunking semantics for the map (e.g. "each chunk carries a disjoint subset of entries; receiver unions in seq order") and add a test that actually populates outputs across multiple messages and reassembles, or narrow the workstream so this field is not chunkable and remove the chunk field from ExecuteResult.

handcaught · 2026-05-16T21:25:13Z

+	if negotiatedMax == 0 {
+		negotiatedMax = DefaultMaxChunkBytes
+	}
+	return uint32(len(data)) > negotiatedMax


uint32(len(data)) silently truncates payloads larger than 4 GiB to the low 32 bits, so a 4 GiB + 1 byte payload reports false for NeedsChunking when negotiatedMax is 4 GiB (it would compare 1 > 4_194_304 and return false). Drop the cast and widen the negotiated max instead: return len(data) > int(negotiatedMax). Same intent, no wraparound.

handcaught · 2026-05-16T21:25:13Z

+// return value) into whatever payload field is being chunked.
+//
+// If len(data) == 0 a single empty chunk is returned.
+func SplitChunks(data []byte, chunkSize uint32) (chunks []*Chunk, payloads [][]byte) {


This helper's signature ([][]byte payloads) only fits one of the three fields the workstream documents as chunkable (LogEvent.line, and only if it becomes bytes — see the comment on adapter.proto:208). For AdapterEvent.payload (Struct) and ExecuteResult.outputs (map<string,string>) there is no caller-side path from [][]byte → typed field, so the helper is effectively unusable for them and gives false confidence that chunking is implemented end-to-end.

Once the field-shape questions on adapter.proto:155 / :173 / :208 are resolved, please either (a) narrow this helper to byte-shaped fields and add separate helpers (or a small interface) for the structured fields, or (b) align all three fields to a bytes-shaped payload so the helper applies uniformly. An integration test that takes a typed value, chunks it, round-trips each chunk through proto.Marshal/Unmarshal, and reassembles is the bar I'd like to see before this lands.

handcaught · 2026-05-16T21:25:13Z

+	assert.Equal(t, payload, reassembled)
+
+	// Wrap in an AdapterEvent and verify proto round-trip preserves chunk metadata.
+	event := &criteriav2.AdapterEvent{


This test proves only that the Chunk envelope round-trips — line 441–446 marshals an AdapterEvent whose only chunk-related field is the envelope and whose payload is unset, and payloads (the byte slices returned by SplitChunks) are never attached to any wire field. The end-to-end contract — chunked bytes traveling on the wire alongside the envelope and reassembling into the typed payload/line/outputs field — is not exercised anywhere in the suite.

Please add a test that, for at least one chunkable field, actually attaches each chunk's payload to a real proto message, marshals, unmarshals, and reassembles the original value. The current assertion gap is what allowed the wire/helper mismatch flagged on chunking.go:35 to ship.

handcaught · 2026-05-16T21:25:13Z

+	sender := func(hb *criteriav2.Heartbeat) error {
+		sent = append(sent, hb)
+		return st.err
+	}


The state struct + closure-captured st.err (lines 22–28) is a roundabout way to write a sender that always returns nil, and the rationale comment ("ensures the linter does not flag the return as 'always nil'") does not match observed golangci-lint behavior — a literal return nil in a test-local closure of type func(*Heartbeat) error is not flagged by the project's linter config.

Please replace lines 22–28 with the direct form:

var sent []*criteriav2.Heartbeat sender := func(hb *criteriav2.Heartbeat) error { sent = append(sent, hb) return nil }

If a linter does object on a future config change, suppress that one line with a brief //nolint: and a reason rather than carrying dead state.

handcaught · 2026-05-16T21:25:13Z

+		t.Run(tc.name, func(t *testing.T) {
+			got := criteriav2.NegotiateChunkSize(tc.adapterMax, tc.hostMax)
+			assert.Equal(t, tc.wantMin, got)
+			assert.Equal(t, tc.wantMax, got)


wantMin and wantMax hold the same value in every row, and lines 30–31 assert got against both — so the table reads as if there is a range under test when there is not. Please collapse to a single want uint32 field and one assert.Equal(t, tc.want, got). This also removes the risk of a future edit silently making the two diverge.