Repository navigation
fix(adcp/v3): keep an empty approved_sellers list on the wire - #593
garvitkaushik-123 wants to merge 1 commit into
Conversation
Plan.ApprovedSellers was tagged omitempty, so an empty list (no seller approved) was dropped on encode exactly like nil. The schema defines null as "any seller", so a plan meant to approve no sellers could reach the governance agent with no seller restriction at all. Drop omitempty on that one field: [] now encodes as [], and nil encodes as null, which the schema already defines as "any seller". The Go type is unchanged. The schema for this field is the same in 3.1, 3.2 and 3.3.0-beta.0. Refs #536 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019tMquArX2EYTUkWaEcowyx
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Checked: drops omitempty on Plan.ApprovedSellers (adcp/v3/governance_types.go:24) so []string{} ("no seller approved") no longer serializes identically to nil ("any seller") — a real fail-open fix on an approval boundary. Schema/type coherence holds: Plan is hand-written and present in both KNOWN_TYPES and EXEMPT, no schema change, no regen owed. Version pathway is correct — field schema is identical ["array","null"] across 3.1/3.2/3.3.0-beta.0, decode stays backward compatible, so fix: (not fix!:) is appropriate. Validate() never reads the field and no fixture does byte comparison, so no collateral breakage. Test covers nil/[]/listed plus round-trip. The nil→null wire/plan_hash shift is flagged by the author and is the schema's canonical encoding.
No critical/high/medium findings. Not gated (gated_paths: false), high_risk false, no author team gate. Rows 1–8 do not fire → row 9 approve.
Problem
Plan.ApprovedSellerswas taggedomitempty, so[]string{}was dropped on encode exactly likenil:nil(any seller)null[]string{}(no seller approved)[][]string{"https://s.example"}["https://s.example"]The schema (
sync-plans-request.json) definesnullas "any seller" and says nothing about an omitted key. A Go buyer sending a plan that approves no sellers could therefore lose that meaning and look unrestricted to the governance agent. Decoding was already correct; only encoding was lossy.Change
adcp/v3/governance_types.go: dropomitemptyonapproved_sellers. The Go type is unchanged, so no caller code breaks.adcp/v3/plan_validate_test.go:TestPlan_ApprovedSellers_EmptyIsNotNullcoversnil,[]and a listed seller on encode, and thatnilversus empty survives a round trip. It fails onmain(theniland[]cases) and passes with the fix.Planis hand-written and in lintEXEMPT, so the generator-levelOptional[T]work in #536 slice B can't reach this field. Slice B will name it as a hand edit; this PR fixes the empty-list case now.Wire note for reviewers
Go buyers that never set
ApprovedSellersnow send"approved_sellers": nullinstead of omitting the key. That is the schema's own encoding of "any seller", so the meaning is unchanged, but the bytes differ. That matters for byte-level payload comparisons and for the JCS-basedplan_hashof those plans. The field's schema is identical in 3.1.29, 3.2.2 and 3.3.0-beta.0 (["array","null"], nominItems), so no version-translation pathway is needed. I've kept this asfix:, notfix!:, because the exported type and the field's meaning are unchanged.Validation
go test ./...inadcp/v3(Go 1.26.2): passgo vet ./...: passgofmt -lon the changed files: cleanpython3 lint.py --strictagainst the pinned 3.2.2 bundle: passgolangci-lintnot run locally: the installed binary was built with Go 1.25 and refuses the 1.26.2 module, so CI covers it.