Skip to content

fix(adcp/v3): keep an empty approved_sellers list on the wire - #594

Open
garvitkaushik-123 wants to merge 1 commit into
mainfrom
fix/approved-sellers-empty-list
Open

garvitkaushik-123 wants to merge 1 commit into
mainfrom
fix/approved-sellers-empty-list

Conversation

@garvitkaushik-123

@garvitkaushik-123 garvitkaushik-123 commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Refs #536 (agreed in the slice 3 design discussion there). Supersedes #593 (same change, moved to a new branch).

Problem

Plan.ApprovedSellers was tagged omitempty, so []string{} was dropped on encode exactly like nil:

Buyer sets Before After
nil (any seller) key omitted null
[]string{} (no seller approved) key omitted []
[]string{"https://s.example"} ["https://s.example"] unchanged

The schema (sync-plans-request.json) defines null as "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: drop omitempty on approved_sellers. The Go type is unchanged, so no caller code breaks.
  • adcp/v3/plan_validate_test.go: TestPlan_ApprovedSellers_EmptyIsNotNull covers nil, [] and a listed seller on encode, and that nil versus empty survives a round trip. It fails on main (the nil and [] cases) and passes with the fix.

Plan is hand-written and in lint EXEMPT, so the generator-level Optional[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 ApprovedSellers now send "approved_sellers": null instead 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-based plan_hash of those plans. The field's schema is identical in 3.1.29, 3.2.2 and 3.3.0-beta.0 (["array","null"], no minItems), so no version-translation pathway is needed. I've kept this as fix:, not fix!:, because the exported type and the field's meaning are unchanged.

Validation

  • go test ./... in adcp/v3 (Go 1.26.2): pass
  • go vet ./...: pass
  • gofmt -l on the changed files: clean
  • python3 lint.py --strict against the pinned 3.2.2 bundle: pass
  • golangci-lint not run locally: the installed binary was built with Go 1.25 and refuses the 1.26.2 module, so CI covers it.

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

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean governance-boundary fix with no blocking, medium, or escalation triggers.

Checked:

  • adcp/v3/governance_types.go: Drops omitempty on Plan.ApprovedSellers so []string{} ("no seller approved") no longer serializes identically to nil ("any seller"). This closes a fail-open on a governance boundary. Verified encode semantics: nil→null, []→[], listed→unchanged.
  • adcp/v3/plan_validate_test.go: New test covers encode plus round-trip across the three cases.
  • Schema↔generated-type coherence: Plan is hand-written, listed in both lint EXEMPT and KNOWN_TYPES, so no regen owed; no adcp/schemas/** touched.
  • Conventional-commit marker: fix: (not fix!:) is justified — exported type and field name/meaning unchanged, only null-vs-empty encoding corrected.
  • Pathway-back (DR-0032/0033): v3 serves 3.0/3.1/3.2 from one struct with identical field schema across minors, so no translation layer is needed; test accompanies the change.
  • Sibling root adcp v2 module with the same bug is frozen/out of scope.

high_risk is false, gated_paths is false, no author team gate, no prior decision. Note: review_decision is REVIEW_REQUIRED, but row 2 only applies when gated_paths is true (it is false here), so no escalation is triggered. Zero findings → falls through to row 9.

@torbenbrodt torbenbrodt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @garvitkaushik-123, this is the fix we agreed on #536, and it looks right to me.

I checked:

  • approved_sellers is ["array", "null"] with "null means any seller" in governance/sync-plans-request.json at both v3.2.2 and adcp main, so null for nil and [] for an empty list is the schema's own encoding.
  • Plan is only reached through SyncPlansRequest in adcp/v3, so no other payload changes shape.
  • TestPlan_ApprovedSellers_EmptyIsNotNull passes on a4d7be5 and fails with omitempty put back (Go 1.26), so it guards the fix.

The 3.1 module has the same bug: adcp/governance_types.go:24 still tags ApprovedSellers omitempty, and its SyncPlansRequest carries []Plan the same way. #554 fixed both modules together; would you add the one-line change and the test there too, or should it be a follow-up?

One non-blocking note beyond your wire note: a buyer that retries a sync_plans request with the same idempotency_key across an SDK upgrade now sends null where it used to omit the key, so a seller hashing the payload (JCS) sees a different request. That's a narrow window and the meaning is unchanged, so I'd keep fix:.

The branch is behind main, so it needs an update before it can merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants