diff --git a/adcp/account_ref_test.go b/adcp/account_ref_test.go new file mode 100644 index 00000000..279fa9bc --- /dev/null +++ b/adcp/account_ref_test.go @@ -0,0 +1,225 @@ +package adcp + +import ( + "context" + "strings" + "testing" +) + +// The v1 resolveAccount guard mirrors v3: an account_id-only reference names +// an account and must reach the resolver instead of silently running with no +// account. See adcontextprotocol/adcp-go#547 and the review of #554. +func TestResolveAccountIDOnlyGoesToResolverV1(t *testing.T) { + var got AccountReference + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + got = ref + return "acct-1", nil + } + + acct, result := resolveAccount(context.Background(), resolver, AccountReference{AccountID: "acc_123"}) + if result != nil || acct != "acct-1" || got.AccountID != "acc_123" { + t.Fatalf("account_id-only reference must reach the resolver (acct=%v, result=%v, got=%+v)", acct, result, got) + } +} + +func TestResolveAccountIDOnlyUnknownYieldsNotFoundV1(t *testing.T) { + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + return nil, nil + } + + _, result := resolveAccount(context.Background(), resolver, AccountReference{AccountID: "acc_nope"}) + if result == nil || !result.IsError { + t.Fatal("expected ACCOUNT_NOT_FOUND error result for unknown account_id") + } + wire, ok := result.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("expected map wire shape, got %T", result.StructuredContent) + } + payload, ok := wire["adcp_error"].(map[string]any) + if !ok { + t.Fatalf("expected adcp_error payload, got %v", wire) + } + if payload["code"] != "ACCOUNT_NOT_FOUND" { + t.Fatalf("expected ACCOUNT_NOT_FOUND, got %v", payload["code"]) + } + if s, _ := payload["suggestion"].(string); !strings.Contains(s, "list_accounts") { + t.Fatalf("account_id suggestion must point at list_accounts, got %q", s) + } +} + +// The v1 package enforces the same pinned-schema validation as v3: a +// supplied reference matching neither account-ref form is rejected with +// INVALID_REQUEST before the resolver runs. +func TestResolveAccountInvalidFormsRejectedV1(t *testing.T) { + cases := map[string]AccountReference{ + "operator only": {Operator: "acme-corp.com"}, + "brand only": {Brand: &BrandReference{Domain: "acme-corp.com"}}, + "account_id + operator": {AccountID: "acc_123", Operator: "acme-corp.com"}, + } + for name, ref := range cases { + t.Run(name, func(t *testing.T) { + called := false + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + called = true + return "acct-1", nil + } + acct, result := resolveAccount(context.Background(), resolver, ref) + if called { + t.Fatal("resolver must not be invoked for a schema-invalid reference") + } + if acct != nil || result == nil || !result.IsError { + t.Fatalf("expected INVALID_REQUEST error result (acct=%v, result=%v)", acct, result) + } + wire, ok := result.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("expected map wire shape, got %T", result.StructuredContent) + } + payload, ok := wire["adcp_error"].(map[string]any) + if !ok { + t.Fatalf("expected adcp_error payload, got %v", wire) + } + if payload["code"] != "INVALID_REQUEST" { + t.Fatalf("expected INVALID_REQUEST, got %v", payload["code"]) + } + }) + } +} + +// TestRegisteredAccountLifecycleV1 mirrors the v3 lifecycle test: the +// combined bug 1 + bug 3 behavior of #547 against the updated v1 SDK, +// through the registered tools, with a lookup-only resolver whose only +// provisioning path is sync_accounts. +func TestRegisteredAccountLifecycleV1(t *testing.T) { + accounts := map[string]*AccountResult{} + deriveID := func(ref AccountReference) string { + if ref.AccountID != "" { + return ref.AccountID + } + domain := "" + if ref.Brand != nil { + domain = ref.Brand.Domain + } + return "acct-" + domain + "-" + ref.Operator + } + cfg := baseTestConfig(Config{ + ResolveAccount: func(_ context.Context, ref AccountReference) (any, error) { + if acct, ok := accounts[deriveID(ref)]; ok { + return acct, nil + } + return nil, nil + }, + SyncAccounts: func(_ context.Context, req *SyncAccountsRequest) ([]AccountResult, error) { + results := make([]AccountResult, 0, len(req.Accounts)) + for _, acct := range req.Accounts { + id := deriveID(AccountReference{Brand: acct.Brand, Operator: acct.Operator}) + result := AccountResult{AccountID: id, Brand: acct.Brand, Operator: acct.Operator, Action: "created", Status: "active"} + if _, ok := accounts[id]; ok { + result.Action = "updated" + } + accounts[id] = &result + results = append(results, result) + } + return results, nil + }, + GetProducts: func(_ context.Context, _ any, _ *GetProductsRequest) (*ProductsData, error) { + return &ProductsData{Products: []Product{}}, nil + }, + }) + + codeOf := func(args map[string]any) string { + // buying_mode is required by get-products-request.json. + args["buying_mode"] = "brief" + result := callRegisteredTool(t, cfg, "get_products", args) + if !result.IsError { + return "" + } + payload, ok := structuredContentMap(t, result)["adcp_error"].(map[string]any) + if !ok { + t.Fatalf("expected adcp_error payload for %v", args) + } + code, _ := payload["code"].(string) + return code + } + + naturalKey := map[string]any{"brand": map[string]any{"domain": "acme-corp.com"}, "operator": "acme-corp.com"} + + if code := codeOf(map[string]any{"account": map[string]any{"account_id": "acc_nope"}}); code != "ACCOUNT_NOT_FOUND" { + t.Fatalf("unknown account_id: want ACCOUNT_NOT_FOUND, got %q", code) + } + if code := codeOf(map[string]any{"account": naturalKey}); code != "ACCOUNT_NOT_FOUND" { + t.Fatalf("unknown natural key: want ACCOUNT_NOT_FOUND, got %q", code) + } + if len(accounts) != 0 { + t.Fatalf("discovery calls must not provision accounts, got %d", len(accounts)) + } + if code := codeOf(map[string]any{"account": map[string]any{"operator": "acme-corp.com"}}); code != "INVALID_REQUEST" { + t.Fatalf("operator-only reference: want INVALID_REQUEST, got %q", code) + } + + // Provisioning-mode entries carry billing (sync-accounts-request.json). + syncEntry := map[string]any{"brand": map[string]any{"domain": "acme-corp.com"}, "operator": "acme-corp.com", "billing": "operator"} + syncResult := callRegisteredTool(t, cfg, "sync_accounts", map[string]any{ + "idempotency_key": "lifecycle-sync-1", + "accounts": []any{syncEntry}, + }) + if syncResult.IsError { + t.Fatalf("sync_accounts must provision, got %v", structuredContentMap(t, syncResult)) + } + if code := codeOf(map[string]any{"account": naturalKey}); code != "" { + t.Fatalf("provisioned natural key must resolve, got error %q", code) + } + if code := codeOf(map[string]any{"account": map[string]any{"account_id": "acct-acme-corp.com-acme-corp.com"}}); code != "" { + t.Fatalf("provisioned account_id must resolve, got error %q", code) + } +} + +// Schema edges from the pinned bundle (review of #554): the account_id arm +// of core/account-ref.json is additionalProperties:false, so sandbox may +// not accompany account_id — while the natural-key arm carries sandbox as +// a modifier (the schema's own fifth example). A present brand_id must +// match core/brand-id.json (^[a-z0-9_]+$). +func TestResolveAccountSchemaEdgeFormsV1(t *testing.T) { + invalid := map[string]AccountReference{ + "account_id + sandbox": {AccountID: "acc_123", Sandbox: true}, + "brand_id with capitals": {Brand: &BrandReference{Domain: "nova-brands.com", BrandID: "Spark"}, Operator: "pinnacle-media.com"}, + "brand_id with dash": {Brand: &BrandReference{Domain: "nova-brands.com", BrandID: "spark-1"}, Operator: "pinnacle-media.com"}, + } + for name, ref := range invalid { + t.Run("invalid/"+name, func(t *testing.T) { + called := false + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + called = true + return "acct-1", nil + } + _, result := resolveAccount(context.Background(), resolver, ref) + if called { + t.Fatal("resolver must not be invoked for a schema-invalid reference") + } + if result == nil || !result.IsError { + t.Fatalf("expected INVALID_REQUEST error result, got %v", result) + } + wire, _ := result.StructuredContent.(map[string]any) + payload, _ := wire["adcp_error"].(map[string]any) + if payload["code"] != "INVALID_REQUEST" { + t.Fatalf("expected INVALID_REQUEST, got %v", payload["code"]) + } + }) + } + valid := map[string]AccountReference{ + "brand_id valid": {Brand: &BrandReference{Domain: "nova-brands.com", BrandID: "spark"}, Operator: "pinnacle-media.com"}, + "natural key + sandbox": {Brand: &BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com", Sandbox: true}, + } + for name, ref := range valid { + t.Run("valid/"+name, func(t *testing.T) { + called := false + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + called = true + return "acct-1", nil + } + acct, result := resolveAccount(context.Background(), resolver, ref) + if !called || result != nil || acct != "acct-1" { + t.Fatalf("schema-valid reference must reach the resolver (called=%v, acct=%v, result=%v)", called, acct, result) + } + }) + } +} diff --git a/adcp/inputs.go b/adcp/inputs.go index 7566fc48..9c82d082 100644 --- a/adcp/inputs.go +++ b/adcp/inputs.go @@ -131,12 +131,17 @@ func isCreativeAssignmentField(key string) bool { // AccountInput is a single account in a sync_accounts request. type AccountInput struct { - Brand *BrandReference `json:"brand,omitempty"` - Operator string `json:"operator,omitempty"` - Billing string `json:"billing,omitempty"` - PaymentTerms string `json:"payment_terms,omitempty"` - AccountID string `json:"account_id,omitempty"` - Sandbox bool `json:"sandbox,omitempty"` + // Account is the settings-update key shape: an entry carrying an + // AccountRef targets an existing account instead of provisioning one + // from the flat Brand/Operator/Billing fields (exactly one key shape + // per entry — see sync-accounts-request.json). + Account *AccountReference `json:"account,omitempty"` + Brand *BrandReference `json:"brand,omitempty"` + Operator string `json:"operator,omitempty"` + Billing string `json:"billing,omitempty"` + PaymentTerms string `json:"payment_terms,omitempty"` + AccountID string `json:"account_id,omitempty"` + Sandbox bool `json:"sandbox,omitempty"` } // GovernanceAccountInput is a single account in a sync_governance request. diff --git a/adcp/seller.go b/adcp/seller.go index 80ba3ec7..2fd7e1b5 100644 --- a/adcp/seller.go +++ b/adcp/seller.go @@ -3,6 +3,7 @@ package adcp import ( "context" "fmt" + "regexp" "time" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -27,7 +28,32 @@ import ( // adcp.Register(server, adcp.Config{ // IdempotencyReplayTTL: 24 * time.Hour, // ResolveAccount: func(ctx context.Context, ref adcp.AccountReference) (any, error) { -// return db.FindAccount(ref.Brand.Domain, ref.Operator) +// // The SDK validates the reference before this call (see +// // Config.ResolveAccount): exactly one form is set — AccountID, +// // or Brand+Operator. Brand is nil for an account_id reference — +// // branch first, and honor ref.Sandbox. +// var acct *db.Account +// if ref.AccountID != "" { +// acct = db.FindAccountByID(ref.AccountID) +// } else { +// acct = db.FindAccount(ref.Brand.Domain, ref.Operator) +// } +// if acct == nil { +// return nil, nil // unknown: the SDK sends ACCOUNT_NOT_FOUND +// } +// // Authorization is part of the lookup, not an afterthought: +// // the principal your own auth middleware put in ctx (with +// // idempotency.WithPrincipal) must be allowed to act for this +// // account. Read it back with +// // idempotency.PrincipalFromContext(ctx) — this package has no +// // top-level PrincipalFromContext; that symbol exists only in +// // adcp/v3, whose Register populates ctx itself. +// if !acct.Authorizes(idempotency.PrincipalFromContext(ctx)) { +// return nil, adcp.NewError("PERMISSION_DENIED", adcp.ErrorOptions{ +// Message: "The caller may not act for this account.", +// }) +// } +// return acct, nil // }, // GetProducts: func(ctx context.Context, acct any, req *adcp.GetProductsRequest) (*adcp.ProductsData, error) { // return &adcp.ProductsData{Products: catalog.Query(req.Brief)}, nil @@ -338,9 +364,42 @@ type Config struct { // is built from the registered handlers. Capabilities *CapabilitiesData - // ResolveAccount converts an AccountReference (brand + operator) to your - // internal account object. Called automatically before handlers that receive - // an account field. Return nil for unknown accounts (SDK sends ACCOUNT_NOT_FOUND). + // ResolveAccount converts an AccountReference (brand + operator, or a bare + // account_id) to your internal account object. Called automatically before + // handlers that receive an account field. Return nil for unknown accounts + // (SDK sends ACCOUNT_NOT_FOUND). + // + // The SDK validates a supplied reference against the pinned + // core/account-ref.json schema before invoking the resolver, so exactly + // one form is set on the reference the resolver receives: AccountID, or + // Brand and Operator together. Brand is nil for an account_id reference, + // so resolvers must branch on AccountID before dereferencing Brand. A + // supplied reference that matches neither schema form (for example an + // operator without a brand) is rejected with INVALID_REQUEST and never + // reaches the resolver. An omitted account field — including a decoded + // reference carrying no identifier — keeps the no-account path and does + // not invoke the resolver either. Sandbox is a modifier of the + // natural-key form only (the account_id arm of the schema allows no + // additional properties, so account_id with sandbox is rejected above); + // on that form it is passed through for the resolver to honor, not + // treated as an identifier. Two decoded shapes are + // deliberately indistinguishable from omission and take the same + // no-account path: a supplied-but-empty reference ({}, or one carrying + // only sandbox) and account_id: "" — the flattened Go struct cannot + // tell them from an absent account field, so they are not rejected. + // When no account is supplied, handlers run with a nil account + // (public discovery) — that behavior is deliberate pending a + // maintainer decision; sellers that require an account must enforce + // that in their handlers. + // + // Authorization is the resolver's job: check that the authenticated + // caller may act for ref. The caller is available via + // idempotency.PrincipalFromContext(ctx) (package adcp/idempotency), + // after your auth middleware sets it with idempotency.WithPrincipal — + // this package does not populate ctx itself, and it has no top-level + // PrincipalFromContext (that symbol exists only in adcp/v3). You MUST + // make this check; otherwise any caller can name another tenant's + // brand/operator and act on it. ResolveAccount func(ctx context.Context, ref AccountReference) (any, error) // --- Media buy --- @@ -386,6 +445,43 @@ type ListCollectionListsResult struct { Pagination *PaginationResponse } +// accountDomainPattern is the domain pattern the pinned schemas apply to +// both core/account-ref.json's operator and core/brand-ref.json's domain. +var accountDomainPattern = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$`) + +// brandIDPattern is the pattern core/brand-id.json applies to brand_id. +var brandIDPattern = regexp.MustCompile(`^[a-z0-9_]+$`) + +// validAccountReference reports whether a supplied (identifier-bearing) +// AccountReference matches one of the two forms in the pinned +// core/account-ref.json schema: {account_id} alone, or the natural key +// {brand, operator} with a schema-valid brand domain and operator. It adds +// no rules of its own beyond the schema. The schema's oneOf arms are +// additionalProperties:false, so sandbox — a property of the natural-key +// arm only — may not accompany account_id, while on the natural-key form +// it stays a modifier passed through for the resolver to honor. When +// brand_id is present it must match core/brand-id.json. References +// carrying no identifier at all never reach this function; resolveAccount +// keeps them on the omitted-account path. +func validAccountReference(ref AccountReference) bool { + if ref.AccountID != "" { + // account_id form: nothing else may accompany it — no natural-key + // fields, and no sandbox (the schema's account_id arm allows no + // additional properties). + return ref.Brand == nil && ref.Operator == "" && !ref.Sandbox + } + // Natural-key form: brand and operator are both required, and both + // domains must match the schema pattern (brand-ref requires domain). + if ref.Brand == nil || ref.Operator == "" { + return false + } + if ref.Brand.BrandID != "" && !brandIDPattern.MatchString(ref.Brand.BrandID) { + return false + } + return accountDomainPattern.MatchString(ref.Brand.Domain) && + accountDomainPattern.MatchString(ref.Operator) +} + func resolveAccount(ctx context.Context, resolver func(context.Context, AccountReference) (any, error), ref any) (any, *mcp.CallToolResult) { if resolver == nil { return nil, nil @@ -404,19 +500,49 @@ func resolveAccount(ctx context.Context, resolver func(context.Context, AccountR result, _, _ := Errorf("INTERNAL_ERROR", ErrorOptions{Message: "unexpected account reference type"}) return nil, result } - if acctRef.Brand == nil && acctRef.Operator == "" { + // An account_id-only reference still names an account: it must go to the + // resolver so an unknown ID surfaces ACCOUNT_NOT_FOUND instead of + // silently running with no account (public pricing). + // + // A decoded reference carrying no identifier at all — a supplied {} + // or {sandbox: true}, or an account_id that decoded to "" — is + // deliberately treated as an omitted account field here: the + // flattened Go struct cannot distinguish it from omission, so it + // takes the same no-account path and never reaches the resolver. + // See the Config.ResolveAccount contract. + if acctRef.Brand == nil && acctRef.Operator == "" && acctRef.AccountID == "" { return nil, nil } + // A supplied reference must match one of the pinned schema's two forms + // before it reaches the resolver: an operator-only reference would + // otherwise arrive with a nil Brand and crash resolvers that dereference + // it. See validAccountReference. + if !validAccountReference(acctRef) { + result, _, _ := Errorf("INVALID_REQUEST", ErrorOptions{ + Message: "Invalid account reference.", + Field: "account", + Suggestion: "Send either an account_id, or a brand and operator together.", + }) + return nil, result + } acct, err := resolver(ctx, acctRef) if err != nil { result, _, _ := errorToResult(err) return nil, result } if acct == nil { + msg := "Account not found. Call sync_accounts first to register this brand and operator." + suggestion := "Call sync_accounts with the brand and operator before making this request." + if acctRef.AccountID != "" { + // Per enums/error-code.json: for an account_id, the fix is to + // verify the ID, not to re-register a brand/operator pair. + msg = "Account not found." + suggestion = "Verify the account_id via list_accounts, or contact the seller." + } result, _, _ := Errorf("ACCOUNT_NOT_FOUND", ErrorOptions{ - Message: "Account not found. Call sync_accounts first to register this brand and operator.", + Message: msg, Field: "account", - Suggestion: "Call sync_accounts with the brand and operator before making this request.", + Suggestion: suggestion, }) return nil, result } diff --git a/adcp/v3/account_ref_test.go b/adcp/v3/account_ref_test.go new file mode 100644 index 00000000..29c535ec --- /dev/null +++ b/adcp/v3/account_ref_test.go @@ -0,0 +1,378 @@ +package adcp + +import ( + "context" + "errors" + "strings" + "testing" +) + +// An account_id-only AccountReference must be handed to the resolver so an +// unknown ID yields ACCOUNT_NOT_FOUND instead of silently running with no +// account. See adcontextprotocol/adcp-go#547. +func TestResolveAccountIDOnlyGoesToResolver(t *testing.T) { + var got AccountReference + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + got = ref + return "acct-1", nil + } + + acct, result := resolveAccount(context.Background(), resolver, AccountReference{AccountID: "acc_123"}) + if result != nil { + t.Fatalf("expected nil result, got %+v", result) + } + if acct != "acct-1" { + t.Fatalf("expected resolved account, got %v", acct) + } + if got.AccountID != "acc_123" { + t.Fatalf("resolver saw wrong reference: %+v", got) + } +} + +func TestResolveAccountIDOnlyPointerGoesToResolver(t *testing.T) { + called := false + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + called = true + return "acct-1", nil + } + + acct, result := resolveAccount(context.Background(), resolver, &AccountReference{AccountID: "acc_123"}) + if result != nil || acct != "acct-1" || !called { + t.Fatalf("account_id-only *AccountReference must reach the resolver (acct=%v, result=%v, called=%v)", acct, result, called) + } +} + +func TestResolveAccountIDOnlyUnknownYieldsNotFound(t *testing.T) { + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + return nil, nil + } + + _, result := resolveAccount(context.Background(), resolver, AccountReference{AccountID: "acc_nope"}) + if result == nil { + t.Fatal("expected ACCOUNT_NOT_FOUND result for unknown account_id") + } + if !result.IsError { + t.Fatal("expected IsError on unknown account_id result") + } + wire, ok := result.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("expected map wire shape, got %T", result.StructuredContent) + } + payload, ok := wire["adcp_error"].(map[string]any) + if !ok { + t.Fatalf("expected adcp_error payload, got %v", wire) + } + if payload["code"] != "ACCOUNT_NOT_FOUND" { + t.Fatalf("expected ACCOUNT_NOT_FOUND, got %v", payload["code"]) + } +} + +func TestResolveAccountEmptyRefIsStillNoAccount(t *testing.T) { + // These are the decoded representations of a request whose account field + // was omitted (or carried no identifier), not schema wire payloads. + cases := map[string]any{ + "nil pointer": (*AccountReference)(nil), + "empty value": AccountReference{}, + "sandbox flag only": AccountReference{Sandbox: true}, + // {"account_id": ""} is schema-valid but names nothing. + "empty account_id": AccountReference{AccountID: ""}, + } + for name, ref := range cases { + t.Run(name, func(t *testing.T) { + called := false + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + called = true + return nil, nil + } + acct, result := resolveAccount(context.Background(), resolver, ref) + if acct != nil || result != nil || called { + t.Fatalf("empty account reference must stay the no-account path (acct=%v, result=%v, called=%v)", acct, result, called) + } + }) + } +} + +func TestResolveAccountBrandRefStillResolves(t *testing.T) { + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + return "brand-acct", nil + } + // The schema's natural-key example: brand.domain + operator (account-ref.json). + ref := AccountReference{Brand: &BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"} + acct, result := resolveAccount(context.Background(), resolver, ref) + if result != nil || acct != "brand-acct" { + t.Fatalf("brand-only reference must keep resolving (acct=%v, result=%v)", acct, result) + } +} + +func TestResolveAccountResolverErrorSurfaces(t *testing.T) { + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + return nil, errors.New("boom") + } + _, result := resolveAccount(context.Background(), resolver, AccountReference{AccountID: "acc_123"}) + if result == nil || !result.IsError { + t.Fatal("resolver errors must surface as an error result") + } +} + +func TestResolveAccountNilResolverIsNoAccount(t *testing.T) { + acct, result := resolveAccount(context.Background(), nil, AccountReference{AccountID: "acc_123"}) + if acct != nil || result != nil { + t.Fatalf("nil resolver must keep the no-account path (acct=%v, result=%v)", acct, result) + } +} + +// The ACCOUNT_NOT_FOUND guidance must match the reference form: an unknown +// account_id points at list_accounts (and the error is terminal, per +// enums/error-code.json), while a natural key points at sync_accounts. +func TestResolveAccountNotFoundGuidanceBranchesOnForm(t *testing.T) { + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + return nil, nil + } + payloadOf := func(ref AccountReference) map[string]any { + _, result := resolveAccount(context.Background(), resolver, ref) + if result == nil { + t.Fatal("expected ACCOUNT_NOT_FOUND result") + } + wire, ok := result.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("expected map wire shape, got %T", result.StructuredContent) + } + payload, ok := wire["adcp_error"].(map[string]any) + if !ok { + t.Fatalf("expected adcp_error payload, got %v", wire) + } + return payload + } + + byID := payloadOf(AccountReference{AccountID: "acc_nope"}) + if byID["recovery"] != "terminal" { + t.Fatalf("account_id ACCOUNT_NOT_FOUND must be terminal, got %v", byID["recovery"]) + } + if s, _ := byID["suggestion"].(string); !strings.Contains(s, "list_accounts") { + t.Fatalf("account_id suggestion must point at list_accounts, got %q", s) + } + + byKey := payloadOf(AccountReference{Brand: &BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"}) + if s, _ := byKey["suggestion"].(string); !strings.Contains(s, "sync_accounts") { + t.Fatalf("natural-key suggestion must point at sync_accounts, got %q", s) + } +} + +// A supplied reference that matches neither form in the pinned +// core/account-ref.json schema must be rejected with INVALID_REQUEST +// before the resolver runs — in particular an operator-only reference must +// never reach a resolver with a nil Brand. See #547 / the #554 hold. +func TestResolveAccountInvalidFormsRejected(t *testing.T) { + cases := map[string]AccountReference{ + "operator only": {Operator: "acme-corp.com"}, + "brand only": {Brand: &BrandReference{Domain: "acme-corp.com"}}, + "brand without domain": {Brand: &BrandReference{BrandID: "spark"}, Operator: "acme-corp.com"}, + "account_id + operator": {AccountID: "acc_123", Operator: "acme-corp.com"}, + "account_id + brand": {AccountID: "acc_123", Brand: &BrandReference{Domain: "acme-corp.com"}}, + "account_id + key": {AccountID: "acc_123", Brand: &BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"}, + "operator not a domain": {Brand: &BrandReference{Domain: "acme-corp.com"}, Operator: "Acme_Corp!"}, + "brand domain invalid": {Brand: &BrandReference{Domain: "-acme-.com"}, Operator: "acme-corp.com"}, + } + for name, ref := range cases { + t.Run(name, func(t *testing.T) { + called := false + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + called = true + return "acct-1", nil + } + acct, result := resolveAccount(context.Background(), resolver, ref) + if called { + t.Fatal("resolver must not be invoked for a schema-invalid reference") + } + if acct != nil || result == nil || !result.IsError { + t.Fatalf("expected INVALID_REQUEST error result (acct=%v, result=%v)", acct, result) + } + wire, ok := result.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("expected map wire shape, got %T", result.StructuredContent) + } + payload, ok := wire["adcp_error"].(map[string]any) + if !ok { + t.Fatalf("expected adcp_error payload, got %v", wire) + } + if payload["code"] != "INVALID_REQUEST" { + t.Fatalf("expected INVALID_REQUEST, got %v", payload["code"]) + } + if payload["recovery"] != "correctable" { + t.Fatalf("INVALID_REQUEST recovery must be correctable, got %v", payload["recovery"]) + } + }) + } +} + +// Sandbox is a modifier of the natural-key form, not an identifier: on +// that form it does not change which schema form a reference is, and a +// sandbox-bearing natural-key reference still reaches the resolver (which +// is responsible for sandbox semantics). Sandbox accompanying account_id +// is schema-invalid — the account_id arm allows no additional properties — +// and is rejected; see TestResolveAccountSchemaEdgeForms. +func TestResolveAccountSandboxFormsStillResolve(t *testing.T) { + cases := map[string]AccountReference{ + "natural key + sandbox": { + Brand: &BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com", Sandbox: true, + }, + } + for name, ref := range cases { + t.Run(name, func(t *testing.T) { + var got AccountReference + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + got = ref + return "acct-1", nil + } + acct, result := resolveAccount(context.Background(), resolver, ref) + if result != nil || acct != "acct-1" || !got.Sandbox { + t.Fatalf("sandbox-bearing valid form must reach the resolver (acct=%v, result=%v, got=%+v)", acct, result, got) + } + }) + } +} + +// TestRegisteredAccountLifecycle exercises the combined bug 1 + bug 3 +// behavior of #547 against the updated SDK, through the registered tools, +// with a resolver written the way the reference sellers now are: lookup +// only, provisioning exclusively via sync_accounts. +// +// - an unknown account_id and an unknown natural key both answer +// ACCOUNT_NOT_FOUND, and the discovery call creates nothing; +// - a schema-invalid (operator-only) reference answers INVALID_REQUEST; +// - after sync_accounts provisions the account, get_products succeeds +// by natural key and by the returned account_id. +func TestRegisteredAccountLifecycle(t *testing.T) { + accounts := map[string]*AccountResult{} + deriveID := func(ref AccountReference) string { + if ref.AccountID != "" { + return ref.AccountID + } + domain := "" + if ref.Brand != nil { + domain = ref.Brand.Domain + } + return "acct-" + domain + "-" + ref.Operator + } + cfg := baseTestConfig(Config{ + ResolveAccount: func(_ context.Context, ref AccountReference) (any, error) { + if acct, ok := accounts[deriveID(ref)]; ok { + return acct, nil + } + return nil, nil + }, + SyncAccounts: func(_ context.Context, req *SyncAccountsRequest) ([]AccountResult, error) { + results := make([]AccountResult, 0, len(req.Accounts)) + for _, acct := range req.Accounts { + id := deriveID(AccountReference{Brand: acct.Brand, Operator: acct.Operator}) + result := AccountResult{AccountID: id, Brand: acct.Brand, Operator: acct.Operator, Action: "created", Status: "active"} + if _, ok := accounts[id]; ok { + result.Action = "updated" + } + accounts[id] = &result + results = append(results, result) + } + return results, nil + }, + GetProducts: func(_ context.Context, _ any, _ *GetProductsRequest) (*ProductsData, error) { + return &ProductsData{Products: []Product{}}, nil + }, + }) + + codeOf := func(args map[string]any) string { + // buying_mode is required by get-products-request.json. + args["buying_mode"] = "brief" + result := callRegisteredTool(t, cfg, "get_products", args) + if !result.IsError { + return "" + } + payload, ok := structuredContentMap(t, result)["adcp_error"].(map[string]any) + if !ok { + t.Fatalf("expected adcp_error payload for %v", args) + } + code, _ := payload["code"].(string) + return code + } + + naturalKey := map[string]any{"brand": map[string]any{"domain": "acme-corp.com"}, "operator": "acme-corp.com"} + + if code := codeOf(map[string]any{"account": map[string]any{"account_id": "acc_nope"}}); code != "ACCOUNT_NOT_FOUND" { + t.Fatalf("unknown account_id: want ACCOUNT_NOT_FOUND, got %q", code) + } + if code := codeOf(map[string]any{"account": naturalKey}); code != "ACCOUNT_NOT_FOUND" { + t.Fatalf("unknown natural key: want ACCOUNT_NOT_FOUND, got %q", code) + } + if len(accounts) != 0 { + t.Fatalf("discovery calls must not provision accounts, got %d", len(accounts)) + } + if code := codeOf(map[string]any{"account": map[string]any{"operator": "acme-corp.com"}}); code != "INVALID_REQUEST" { + t.Fatalf("operator-only reference: want INVALID_REQUEST, got %q", code) + } + + // Provisioning-mode entries carry billing (sync-accounts-request.json). + syncEntry := map[string]any{"brand": map[string]any{"domain": "acme-corp.com"}, "operator": "acme-corp.com", "billing": "operator"} + syncResult := callRegisteredTool(t, cfg, "sync_accounts", map[string]any{ + "idempotency_key": "lifecycle-sync-1", + "accounts": []any{syncEntry}, + }) + if syncResult.IsError { + t.Fatalf("sync_accounts must provision, got %v", structuredContentMap(t, syncResult)) + } + if code := codeOf(map[string]any{"account": naturalKey}); code != "" { + t.Fatalf("provisioned natural key must resolve, got error %q", code) + } + if code := codeOf(map[string]any{"account": map[string]any{"account_id": "acct-acme-corp.com-acme-corp.com"}}); code != "" { + t.Fatalf("provisioned account_id must resolve, got error %q", code) + } +} + +// Schema edges from the pinned bundle (review of #554): the account_id arm +// of core/account-ref.json is additionalProperties:false, so sandbox may +// not accompany account_id — while the natural-key arm carries sandbox as +// a modifier (the schema's own fifth example). A present brand_id must +// match core/brand-id.json (^[a-z0-9_]+$). +func TestResolveAccountSchemaEdgeForms(t *testing.T) { + invalid := map[string]AccountReference{ + "account_id + sandbox": {AccountID: "acc_123", Sandbox: true}, + "brand_id with capitals": {Brand: &BrandReference{Domain: "nova-brands.com", BrandID: "Spark"}, Operator: "pinnacle-media.com"}, + "brand_id with dash": {Brand: &BrandReference{Domain: "nova-brands.com", BrandID: "spark-1"}, Operator: "pinnacle-media.com"}, + } + for name, ref := range invalid { + t.Run("invalid/"+name, func(t *testing.T) { + called := false + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + called = true + return "acct-1", nil + } + _, result := resolveAccount(context.Background(), resolver, ref) + if called { + t.Fatal("resolver must not be invoked for a schema-invalid reference") + } + if result == nil || !result.IsError { + t.Fatalf("expected INVALID_REQUEST error result, got %v", result) + } + wire, _ := result.StructuredContent.(map[string]any) + payload, _ := wire["adcp_error"].(map[string]any) + if payload["code"] != "INVALID_REQUEST" { + t.Fatalf("expected INVALID_REQUEST, got %v", payload["code"]) + } + }) + } + valid := map[string]AccountReference{ + "brand_id valid": {Brand: &BrandReference{Domain: "nova-brands.com", BrandID: "spark"}, Operator: "pinnacle-media.com"}, + "natural key + sandbox": {Brand: &BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com", Sandbox: true}, + } + for name, ref := range valid { + t.Run("valid/"+name, func(t *testing.T) { + called := false + resolver := func(ctx context.Context, ref AccountReference) (any, error) { + called = true + return "acct-1", nil + } + acct, result := resolveAccount(context.Background(), resolver, ref) + if !called || result != nil || acct != "acct-1" { + t.Fatalf("schema-valid reference must reach the resolver (called=%v, acct=%v, result=%v)", called, acct, result) + } + }) + } +} diff --git a/adcp/v3/errors.go b/adcp/v3/errors.go index dddaa96d..deedf2f9 100644 --- a/adcp/v3/errors.go +++ b/adcp/v3/errors.go @@ -112,8 +112,12 @@ func defaultRecovery(code string) string { case "RATE_LIMITED", "SERVICE_UNAVAILABLE": return "transient" case "BUDGET_TOO_LOW", "INVALID_REQUEST", "MISSING_FIELD", "INVALID_FIELD", - "ACCOUNT_NOT_FOUND", "TERMS_REJECTED": + "TERMS_REJECTED": return "correctable" + case "ACCOUNT_NOT_FOUND": + // enums/error-code.json classifies ACCOUNT_NOT_FOUND as terminal; + // matches the v1 module (adcp/errors.go). + return "terminal" default: return "terminal" } diff --git a/adcp/v3/inputs.go b/adcp/v3/inputs.go index 2081dd64..84d67abf 100644 --- a/adcp/v3/inputs.go +++ b/adcp/v3/inputs.go @@ -207,12 +207,17 @@ func isCreativeAssignmentField(key string) bool { // AccountInput is a single account in a sync_accounts request. type AccountInput struct { - Brand *BrandReference `json:"brand,omitempty"` - Operator string `json:"operator,omitempty"` - Billing string `json:"billing,omitempty"` - PaymentTerms string `json:"payment_terms,omitempty"` - AccountID string `json:"account_id,omitempty"` - Sandbox bool `json:"sandbox,omitempty"` + // Account is the settings-update key shape: an entry carrying an + // AccountRef targets an existing account instead of provisioning one + // from the flat Brand/Operator/Billing fields (exactly one key shape + // per entry — see sync-accounts-request.json). + Account *AccountReference `json:"account,omitempty"` + Brand *BrandReference `json:"brand,omitempty"` + Operator string `json:"operator,omitempty"` + Billing string `json:"billing,omitempty"` + PaymentTerms string `json:"payment_terms,omitempty"` + AccountID string `json:"account_id,omitempty"` + Sandbox bool `json:"sandbox,omitempty"` } // GovernanceAccountInput is a single account in a sync_governance request. diff --git a/adcp/v3/schemas/lint.py b/adcp/v3/schemas/lint.py index 4b16e1db..dc07d074 100644 --- a/adcp/v3/schemas/lint.py +++ b/adcp/v3/schemas/lint.py @@ -131,7 +131,6 @@ KNOWN_SCHEMA_GAPS = { 'AccountInput': { 'missing_in_go': { - 'account': '#564', 'billing_entity': '#564', 'currency': '#564', 'destination_billing_entity': '#564', diff --git a/adcp/v3/seller.go b/adcp/v3/seller.go index fea724b7..b5ba62cf 100644 --- a/adcp/v3/seller.go +++ b/adcp/v3/seller.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "regexp" "time" "github.com/adcontextprotocol/adcp-go/adcp/v3/idempotency" @@ -34,7 +35,27 @@ import ( // IdempotencyReplayTTL: 24 * time.Hour, // Idempotency: store, // optional; idempotency.New(...) built once, outside the server factory // ResolveAccount: func(ctx context.Context, ref adcp.AccountReference) (any, error) { -// return db.FindAccount(ref.Brand.Domain, ref.Operator) +// // The SDK validates the reference before this call (see +// // Config.ResolveAccount): exactly one form is set — AccountID, +// // or Brand+Operator. Brand is nil for an account_id reference — +// // branch first, and honor ref.Sandbox. +// var acct *db.Account +// if ref.AccountID != "" { +// acct = db.FindAccountByID(ref.AccountID) +// } else { +// acct = db.FindAccount(ref.Brand.Domain, ref.Operator) +// } +// if acct == nil { +// return nil, nil // unknown: the SDK sends ACCOUNT_NOT_FOUND +// } +// // Authorization is part of the lookup, not an afterthought: +// // the caller principal must be allowed to act for this account. +// if !acct.Authorizes(adcp.PrincipalFromContext(ctx)) { +// return nil, adcp.NewError("PERMISSION_DENIED", adcp.ErrorOptions{ +// Message: "The caller may not act for this account.", +// }) +// } +// return acct, nil // }, // GetProducts: func(ctx context.Context, acct any, req *adcp.GetProductsRequest) (*adcp.ProductsData, error) { // return &adcp.ProductsData{Products: catalog.Query(req.Brief)}, nil @@ -505,9 +526,33 @@ type Config struct { // is built from the registered handlers. Capabilities *CapabilitiesData - // ResolveAccount converts an AccountReference (brand + operator) to your - // internal account object. Called automatically before handlers that receive - // an account field. Return nil for unknown accounts (SDK sends ACCOUNT_NOT_FOUND). + // ResolveAccount converts an AccountReference (brand + operator, or a bare + // account_id) to your internal account object. Called automatically before + // handlers that receive an account field. Return nil for unknown accounts + // (SDK sends ACCOUNT_NOT_FOUND). + // + // The SDK validates a supplied reference against the pinned + // core/account-ref.json schema before invoking the resolver, so exactly + // one form is set on the reference the resolver receives: AccountID, or + // Brand and Operator together. Brand is nil for an account_id reference, + // so resolvers must branch on AccountID before dereferencing Brand. A + // supplied reference that matches neither schema form (for example an + // operator without a brand) is rejected with INVALID_REQUEST and never + // reaches the resolver. An omitted account field — including a decoded + // reference carrying no identifier — keeps the no-account path and does + // not invoke the resolver either. Sandbox is a modifier of the + // natural-key form only (the account_id arm of the schema allows no + // additional properties, so account_id with sandbox is rejected above); + // on that form it is passed through for the resolver to honor, not + // treated as an identifier. Two decoded shapes are + // deliberately indistinguishable from omission and take the same + // no-account path: a supplied-but-empty reference ({}, or one carrying + // only sandbox) and account_id: "" — the flattened Go struct cannot + // tell them from an absent account field, so they are not rejected. + // When no account is supplied, handlers run with a nil account + // (public discovery) — that behavior is deliberate pending a + // maintainer decision; sellers that require an account must enforce + // that in their handlers. // // The authenticated caller is available via PrincipalFromContext(ctx). // You MUST check that this principal may act for ref; otherwise any @@ -594,6 +639,43 @@ type ListCollectionListsResult struct { Pagination *PaginationResponse } +// accountDomainPattern is the domain pattern the pinned schemas apply to +// both core/account-ref.json's operator and core/brand-ref.json's domain. +var accountDomainPattern = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$`) + +// brandIDPattern is the pattern core/brand-id.json applies to brand_id. +var brandIDPattern = regexp.MustCompile(`^[a-z0-9_]+$`) + +// validAccountReference reports whether a supplied (identifier-bearing) +// AccountReference matches one of the two forms in the pinned +// core/account-ref.json schema: {account_id} alone, or the natural key +// {brand, operator} with a schema-valid brand domain and operator. It adds +// no rules of its own beyond the schema. The schema's oneOf arms are +// additionalProperties:false, so sandbox — a property of the natural-key +// arm only — may not accompany account_id, while on the natural-key form +// it stays a modifier passed through for the resolver to honor. When +// brand_id is present it must match core/brand-id.json. References +// carrying no identifier at all never reach this function; resolveAccount +// keeps them on the omitted-account path. +func validAccountReference(ref AccountReference) bool { + if ref.AccountID != "" { + // account_id form: nothing else may accompany it — no natural-key + // fields, and no sandbox (the schema's account_id arm allows no + // additional properties). + return ref.Brand == nil && ref.Operator == "" && !ref.Sandbox + } + // Natural-key form: brand and operator are both required, and both + // domains must match the schema pattern (brand-ref requires domain). + if ref.Brand == nil || ref.Operator == "" { + return false + } + if ref.Brand.BrandID != "" && !brandIDPattern.MatchString(ref.Brand.BrandID) { + return false + } + return accountDomainPattern.MatchString(ref.Brand.Domain) && + accountDomainPattern.MatchString(ref.Operator) +} + func resolveAccount(ctx context.Context, resolver func(context.Context, AccountReference) (any, error), ref any) (any, *mcp.CallToolResult) { if resolver == nil { return nil, nil @@ -612,19 +694,49 @@ func resolveAccount(ctx context.Context, resolver func(context.Context, AccountR result, _, _ := Errorf("INTERNAL_ERROR", ErrorOptions{Message: "unexpected account reference type"}) return nil, result } - if acctRef.Brand == nil && acctRef.Operator == "" { + // An account_id-only reference still names an account: it must go to the + // resolver so an unknown ID surfaces ACCOUNT_NOT_FOUND instead of + // silently running with no account (public pricing). + // + // A decoded reference carrying no identifier at all — a supplied {} + // or {sandbox: true}, or an account_id that decoded to "" — is + // deliberately treated as an omitted account field here: the + // flattened Go struct cannot distinguish it from omission, so it + // takes the same no-account path and never reaches the resolver. + // See the Config.ResolveAccount contract. + if acctRef.Brand == nil && acctRef.Operator == "" && acctRef.AccountID == "" { return nil, nil } + // A supplied reference must match one of the pinned schema's two forms + // before it reaches the resolver: an operator-only reference would + // otherwise arrive with a nil Brand and crash resolvers that dereference + // it. See validAccountReference. + if !validAccountReference(acctRef) { + result, _, _ := Errorf("INVALID_REQUEST", ErrorOptions{ + Message: "Invalid account reference.", + Field: "account", + Suggestion: "Send either an account_id, or a brand and operator together.", + }) + return nil, result + } acct, err := resolver(ctx, acctRef) if err != nil { result, _, _ := errorToResult(err) return nil, result } if acct == nil { + msg := "Account not found. Call sync_accounts first to register this brand and operator." + suggestion := "Call sync_accounts with the brand and operator before making this request." + if acctRef.AccountID != "" { + // Per enums/error-code.json: for an account_id, the fix is to + // verify the ID, not to re-register a brand/operator pair. + msg = "Account not found." + suggestion = "Verify the account_id via list_accounts, or contact the seller." + } result, _, _ := Errorf("ACCOUNT_NOT_FOUND", ErrorOptions{ - Message: "Account not found. Call sync_accounts first to register this brand and operator.", + Message: msg, Field: "account", - Suggestion: "Call sync_accounts with the brand and operator before making this request.", + Suggestion: suggestion, }) return nil, result } diff --git a/reference/hello-seller/README.md b/reference/hello-seller/README.md index 34d5ca34..f473ea0a 100644 --- a/reference/hello-seller/README.md +++ b/reference/hello-seller/README.md @@ -51,6 +51,17 @@ boundary by itself. {"buying_mode": "brief", "brief": "premium display inventory"} ``` +`sync_accounts` — provision the account first. `create_media_buy` below +names this account, and since the reference resolvers are lookup-only an +unprovisioned account returns `ACCOUNT_NOT_FOUND`: + +```json +{ + "idempotency_key": "22222222-2222-2222-2222-222222222222", + "accounts": [{"brand": {"domain": "advertiser.example"}, "operator": "agency.example", "billing": "operator"}] +} +``` + `create_media_buy` (use a `product_id` and `pricing_option_id` from the `get_products` response above): @@ -85,10 +96,11 @@ Every item below is marked `// SWAP:` at its integration point in the source. response as sandbox data. A production seller sets this from real environment/deploy configuration, not a hardcoded literal — same for the `"sandbox": true` in `extensions.go`'s `updateMediaBuyResult`. -2. **`resolveAccount`** — replace the in-memory map + auto-create-on-miss - with a real lookup against your account/CRM/billing system. Return - `(nil, nil)` for a genuinely unknown account (not an error) so the SDK - returns `ACCOUNT_NOT_FOUND` and the buyer calls `sync_accounts` first. +2. **`resolveAccount`** — replace the in-memory map with a real lookup + against your account/CRM/billing system. It already returns `(nil, nil)` + for a genuinely unknown account (not an error) so the SDK returns + `ACCOUNT_NOT_FOUND`; accounts are created only by `sync_accounts` + (the provisioning path in `main.go`), never on a lookup miss. 3. **In-memory maps → durable storage.** `backend`'s maps (`products`, `mediaBuys`, `creatives`) are lost on every restart. Replace with your real ad server / OMS / database. diff --git a/reference/hello-seller/main.go b/reference/hello-seller/main.go index 7e6bc6cf..2e0447c6 100644 --- a/reference/hello-seller/main.go +++ b/reference/hello-seller/main.go @@ -88,9 +88,17 @@ func newBackend() *backend { // internal account object. It's called automatically before every handler // that receives an account field — you never call it yourself. // -// SWAP: look up the real account in your CRM/billing system, and return -// (nil, nil) — not an error — for a genuinely unknown account; the SDK turns -// that into the correct ACCOUNT_NOT_FOUND response for you. +// This is a lookup only: an unknown reference returns (nil, nil) — not an +// error — and the SDK turns that into the correct ACCOUNT_NOT_FOUND +// response. Accounts are created only through the provisioning path +// (sync_accounts, below); a discovery or media-buy call must never mint +// one, or any caller could enumerate/fabricate accounts by naming them. +// +// SWAP: look up the real account in your CRM/billing system instead of the +// in-memory map, keeping the same return-nil-for-unknown contract. In +// production, also check that the caller principal may act for the +// account, and honor ref.Sandbox — a sandbox reference must never resolve +// to a production account. func (b *backend) resolveAccount(_ context.Context, ref adcp.AccountReference) (any, error) { b.mu.Lock() defer b.mu.Unlock() @@ -98,16 +106,44 @@ func (b *backend) resolveAccount(_ context.Context, ref adcp.AccountReference) ( if ref.Brand != nil { domain = ref.Brand.Domain } - id := fmt.Sprintf("acct-%s-%s", domain, ref.Operator) + // Accounts are keyed by the id sync_accounts returned, so an + // account_id reference looks up that id directly; only natural-key + // references derive the id from brand+operator. + id := ref.AccountID + if id == "" { + id = fmt.Sprintf("acct-%s-%s", domain, ref.Operator) + } if acct, ok := b.accounts[id]; ok { return acct, nil } - // Demo-only: silently mint a new account for anything unrecognized. - // SWAP: a real implementation returns (nil, nil) here instead, so the - // buyer gets ACCOUNT_NOT_FOUND and calls sync_accounts first. - acct := &adcp.AccountResult{AccountID: id, Brand: ref.Brand, Operator: ref.Operator, Action: "existing", Status: "active"} - b.accounts[id] = acct - return acct, nil + return nil, nil +} + +// syncAccounts is the provisioning path: the only place accounts are +// created. A buyer calls sync_accounts first, then names the account — by +// the returned account_id or by its natural key — on account-scoped calls. +func (b *backend) syncAccounts(_ context.Context, input *adcp.SyncAccountsRequest) ([]adcp.AccountResult, error) { + b.mu.Lock() + defer b.mu.Unlock() + results := make([]adcp.AccountResult, 0, len(input.Accounts)) + for _, acct := range input.Accounts { + if acct.Brand == nil || acct.Operator == "" { + // No natural key on this entry — e.g. a settings-update entry + // keyed by an `account` ref, which this SDK version's + // AccountInput cannot decode. Never fabricate an account from + // empty fields (the old code minted "acct-unknown-"). + continue + } + id := fmt.Sprintf("acct-%s-%s", acct.Brand.Domain, acct.Operator) + result := adcp.AccountResult{AccountID: id, Brand: acct.Brand, Operator: acct.Operator, Action: "created", Status: "active"} + if existing, ok := b.accounts[id]; ok { + result.Action = "updated" + result.Status = existing.Status + } + b.accounts[id] = &result + results = append(results, result) + } + return results, nil } // getProducts returns the seller's catalog for get_products. SWAP: query @@ -218,6 +254,7 @@ func newServer(b *backend) *mcp.Server { Creative: &adcp.CreativeCapabilities{HasCreativeLibrary: adcp.Bool(true)}, }, ResolveAccount: b.resolveAccount, + SyncAccounts: b.syncAccounts, GetProducts: b.getProducts, CreateMediaBuy: b.createMediaBuy, GetMediaBuys: b.getMediaBuys, diff --git a/reference/hello-seller/main_test.go b/reference/hello-seller/main_test.go index a2fe345e..dcbaaf29 100644 --- a/reference/hello-seller/main_test.go +++ b/reference/hello-seller/main_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "strings" "testing" "github.com/adcontextprotocol/adcp-go/adcp/v3" @@ -165,3 +166,98 @@ func TestNewServer_RegistersWithoutPanicking(t *testing.T) { t.Fatal("expected non-nil server") } } + +// TestResolveAccountUnknownDoesNotProvision pins bug 3 of adcp-go#547: a +// lookup for an unknown account — by natural key or by account_id — must +// return no account and must not create one. Provisioning happens only +// through sync_accounts. +func TestResolveAccountUnknownDoesNotProvision(t *testing.T) { + b := newBackend() + ctx := context.Background() + + refs := map[string]adcp.AccountReference{ + "natural key": {Brand: &adcp.BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"}, + "account_id": {AccountID: "acc_nope"}, + } + for name, ref := range refs { + acct, err := b.resolveAccount(ctx, ref) + if err != nil { + t.Fatalf("%s: resolveAccount: %v", name, err) + } + if acct != nil { + t.Fatalf("%s: unknown account must resolve to nil, got %v", name, acct) + } + } + if len(b.accounts) != 0 { + t.Fatalf("resolution must not create accounts, got %d", len(b.accounts)) + } +} + +// TestSyncAccountsProvisionsThenResolves exercises the combined behavior: +// after sync_accounts provisions an account, resolveAccount finds it by +// natural key and by the returned account_id; a re-sync reports "updated". +func TestSyncAccountsProvisionsThenResolves(t *testing.T) { + b := newBackend() + ctx := context.Background() + + results, err := b.syncAccounts(ctx, &adcp.SyncAccountsRequest{ + Accounts: []adcp.AccountInput{{Brand: &adcp.BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"}}, + }) + if err != nil { + t.Fatalf("syncAccounts: %v", err) + } + if len(results) != 1 || results[0].Action != "created" || results[0].AccountID == "" { + t.Fatalf("want one created account with an id, got %+v", results) + } + id := results[0].AccountID + + byKey, err := b.resolveAccount(ctx, adcp.AccountReference{Brand: &adcp.BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"}) + if err != nil || byKey == nil { + t.Fatalf("provisioned account must resolve by natural key (acct=%v, err=%v)", byKey, err) + } + byID, err := b.resolveAccount(ctx, adcp.AccountReference{AccountID: id}) + if err != nil || byID == nil { + t.Fatalf("provisioned account must resolve by account_id (acct=%v, err=%v)", byID, err) + } + + results, err = b.syncAccounts(ctx, &adcp.SyncAccountsRequest{ + Accounts: []adcp.AccountInput{{Brand: &adcp.BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"}}, + }) + if err != nil { + t.Fatalf("syncAccounts (re-sync): %v", err) + } + if len(results) != 1 || results[0].Action != "updated" { + t.Fatalf("re-sync must report updated, got %+v", results) + } +} + +// Entries without a natural key (no brand, or no operator — e.g. a +// settings-update entry keyed by an `account` ref) must be skipped: +// sync_accounts must never fabricate an "acct-unknown-" account. +func TestSyncAccountsSkipsEntriesWithoutNaturalKey(t *testing.T) { + b := newBackend() + ctx := context.Background() + + results, err := b.syncAccounts(ctx, &adcp.SyncAccountsRequest{ + Accounts: []adcp.AccountInput{ + {}, + {Brand: &adcp.BrandReference{Domain: "acme-corp.com"}}, + {Operator: "acme-corp.com"}, + {Brand: &adcp.BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"}, + }, + }) + if err != nil { + t.Fatalf("syncAccounts: %v", err) + } + if len(results) != 1 { + t.Fatalf("want exactly one provisioned account, got %+v", results) + } + if len(b.accounts) != 1 { + t.Fatalf("want exactly one stored account, got %d: %v", len(b.accounts), b.accounts) + } + for id := range b.accounts { + if strings.HasPrefix(id, "acct-unknown") { + t.Fatalf("fabricated account id %q", id) + } + } +} diff --git a/reference/seller-agent/cmd/seller-agent/main.go b/reference/seller-agent/cmd/seller-agent/main.go index 008f8a20..66158776 100644 --- a/reference/seller-agent/cmd/seller-agent/main.go +++ b/reference/seller-agent/cmd/seller-agent/main.go @@ -539,6 +539,36 @@ func (b *backend) getDelivery(input *adcp.GetMediaBuyDeliveryRequest) (*adcp.Del return &adcp.DeliveryData{ReportingPeriod: adcp.ReportingPeriod{Start: now.Add(-24 * time.Hour).Format(time.RFC3339), End: now.Format(time.RFC3339)}, Currency: "USD", MediaBuyDeliveries: deliveries}, nil } +// resolveAccount is a lookup only: an unknown reference — by account_id or +// by natural key — returns (nil, nil), and the SDK turns that into +// ACCOUNT_NOT_FOUND. Accounts are created only through the provisioning +// path (the SyncAccounts handler in newServer); a discovery or media-buy +// call must never fabricate one, or any caller could enumerate accounts by +// naming them. See adcp-go#547 bug 3. +// +// SWAP: when the in-memory map becomes a real account store, also check +// that the caller principal may act for the account, and honor +// ref.Sandbox — a sandbox reference must never resolve to a production +// account. +func (b *backend) resolveAccount(_ context.Context, ref adcp.AccountReference) (any, error) { + b.mu.RLock() + defer b.mu.RUnlock() + domain := "" + if ref.Brand != nil { + domain = ref.Brand.Domain + } + // An account_id reference names the account sync_accounts + // returned; look that id up before deriving the natural key. + id := ref.AccountID + if id == "" { + id = fmt.Sprintf("acct-%s-%s", domain, ref.Operator) + } + if acct, ok := b.accounts[id]; ok { + return acct, nil + } + return nil, nil +} + func (b *backend) forceAccountStatus(accountID, status string) (*adcp.StateTransition, error) { b.mu.Lock() defer b.mu.Unlock() @@ -1278,29 +1308,21 @@ func newServer(b *backend) *mcp.Server { "simulate_delivery", "simulate_budget_spend", }}, }, - ResolveAccount: func(_ context.Context, ref adcp.AccountReference) (any, error) { - b.mu.RLock() - defer b.mu.RUnlock() - domain := "" - if ref.Brand != nil { - domain = ref.Brand.Domain - } - id := fmt.Sprintf("acct-%s-%s", domain, ref.Operator) - if acct, ok := b.accounts[id]; ok { - return acct, nil - } - return &adcp.AccountResult{AccountID: id, Brand: ref.Brand, Operator: ref.Operator, Action: "existing", Status: "active"}, nil - }, + ResolveAccount: b.resolveAccount, SyncAccounts: func(_ context.Context, input *adcp.SyncAccountsRequest) ([]adcp.AccountResult, error) { b.mu.Lock() defer b.mu.Unlock() results := make([]adcp.AccountResult, 0, len(input.Accounts)) for _, acct := range input.Accounts { - domain := "unknown" - if acct.Brand != nil { - domain = acct.Brand.Domain + if acct.Brand == nil || acct.Operator == "" { + // No natural key on this entry — e.g. a settings-update + // entry keyed by an `account` ref, which this SDK + // version's AccountInput cannot decode. Never fabricate + // an account from empty fields (the old code minted + // "acct-unknown-"). + continue } - id := fmt.Sprintf("acct-%s-%s", domain, acct.Operator) + id := fmt.Sprintf("acct-%s-%s", acct.Brand.Domain, acct.Operator) result := adcp.AccountResult{AccountID: id, Brand: acct.Brand, Operator: acct.Operator, Action: "created", Status: "active"} if existing, ok := b.accounts[id]; ok { result.Action = "updated" @@ -1426,6 +1448,30 @@ func main() { creatives: make(map[string]*creativeRecord), delivery: make(map[string]*deliveryState), } + // Pre-provisioned sandbox account for the storyboard test kit + // (test-kits/acme-outdoor.yaml): the media_buy_seller storyboard names + // this buyer on get_products/create_media_buy without a sync_accounts + // step, so the operator provisions it at startup, like the seeded + // product catalog. Resolution itself never creates accounts — every + // other unknown reference still answers ACCOUNT_NOT_FOUND (#547 bug 3). + b.accounts["acct-acmeoutdoor.example-pinnacle-agency.example"] = &adcp.AccountResult{ + AccountID: "acct-acmeoutdoor.example-pinnacle-agency.example", + Brand: &adcp.BrandReference{Domain: "acmeoutdoor.example"}, + Operator: "pinnacle-agency.example", + Action: "existing", + Status: "active", + } + // The storyboard runner's enricher-built requests (steps without a + // fixture account) use its default buyer account, test.example + // (resolveAccount in @adcp/sdk's storyboard runner). Seed that fixture + // account on the same terms as the test-kit account above. + b.accounts["acct-test.example-test.example"] = &adcp.AccountResult{ + AccountID: "acct-test.example-test.example", + Brand: &adcp.BrandReference{Domain: "test.example"}, + Operator: "test.example", + Action: "existing", + Status: "active", + } log.Fatal(adcp.Serve(func() *mcp.Server { return newServer(b) diff --git a/reference/seller-agent/cmd/seller-agent/main_mcp_test.go b/reference/seller-agent/cmd/seller-agent/main_mcp_test.go index 770c3d0f..043b4532 100644 --- a/reference/seller-agent/cmd/seller-agent/main_mcp_test.go +++ b/reference/seller-agent/cmd/seller-agent/main_mcp_test.go @@ -457,3 +457,83 @@ func TestMCP_TermsRejected_RecoveryIsEnumMember(t *testing.T) { t.Errorf("recovery = %q, want correctable (the buyer can revise the terms and resend)", recovery) } } + +// TestMCP_GetProducts_UnknownAccountNotFound pins the tool-level side of +// adcp-go#547 bug 3: get_products naming an account sync_accounts never +// provisioned must answer ACCOUNT_NOT_FOUND — the resolver must not +// fabricate an active account for the call. After sync_accounts provisions +// the same natural key, the call succeeds. +func TestMCP_GetProducts_UnknownAccountNotFound(t *testing.T) { + session := newMCPTestSession(t) + account := map[string]any{ + "brand": map[string]any{"domain": "acme-corp.com"}, + "operator": "acme-corp.com", + } + + result := callTool(t, session, "get_products", map[string]any{"account": account}) + if !result.IsError { + t.Fatal("expected error result for an unknown account") + } + wire := structuredMap(t, result) + errPayload, ok := wire["adcp_error"].(map[string]any) + if !ok { + t.Fatalf("expected adcp_error payload, got %v", wire) + } + if errPayload["code"] != "ACCOUNT_NOT_FOUND" { + t.Fatalf("expected ACCOUNT_NOT_FOUND, got %v", errPayload["code"]) + } + + syncResult := callTool(t, session, "sync_accounts", map[string]any{ + "idempotency_key": "test-sync-unknown-account", + "accounts": []any{account}, + }) + if syncResult.IsError { + t.Fatalf("sync_accounts must provision the account, got %v", structuredMap(t, syncResult)) + } + result = callTool(t, session, "get_products", map[string]any{"account": account}) + if result.IsError { + t.Fatalf("get_products must succeed after provisioning, got %v", structuredMap(t, result)) + } +} + +// TestMCP_SyncAccounts_SkipsAccountOnlyEntries pins the review-of-#554 +// finding for sync_accounts entries: an entry keyed only by an `account` +// ref (settings-update mode — no brand, no operator, no account_id at the +// entry root) must be skipped, not provisioned as a fabricated +// "acct-unknown-" account. The response counts only the entries actually +// provisioned. +func TestMCP_SyncAccounts_SkipsAccountOnlyEntries(t *testing.T) { + b := newTestBackend() + session := newMCPTestSessionForBackend(t, b) + + syncResult := callTool(t, session, "sync_accounts", map[string]any{ + "idempotency_key": "test-sync-account-only-entry", + "accounts": []any{ + map[string]any{"account": map[string]any{"account_id": "acc_existing"}}, + map[string]any{ + "brand": map[string]any{"domain": "acme-corp.com"}, + "operator": "acme-corp.com", + "billing": "operator", + }, + }, + }) + if syncResult.IsError { + t.Fatalf("sync_accounts must succeed, got %v", structuredMap(t, syncResult)) + } + wire := structuredMap(t, syncResult) + accounts, ok := wire["accounts"].([]any) + if !ok || len(accounts) != 1 { + t.Fatalf("want exactly one provisioned account in the response, got %v", wire["accounts"]) + } + if len(b.accounts) != 1 { + t.Fatalf("want exactly one stored account, got %d: %v", len(b.accounts), b.accounts) + } + for id := range b.accounts { + if id == "acct-unknown-" { + t.Fatalf("fabricated account id %q must not be created", id) + } + } + if _, ok := b.accounts["acct-acme-corp.com-acme-corp.com"]; !ok { + t.Fatalf("the valid entry must be provisioned, got %v", b.accounts) + } +} diff --git a/reference/seller-agent/cmd/seller-agent/main_test.go b/reference/seller-agent/cmd/seller-agent/main_test.go index 371cd34b..98733669 100644 --- a/reference/seller-agent/cmd/seller-agent/main_test.go +++ b/reference/seller-agent/cmd/seller-agent/main_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "encoding/json" "testing" @@ -1070,3 +1071,45 @@ func resultErrorCode(t *testing.T, result *mcp.CallToolResult) string { } return wire.ADCPError.Code } + +// TestResolveAccountUnknownDoesNotFabricate pins bug 3 of adcp-go#547: +// resolveAccount is a lookup only — an unknown account_id or natural key +// returns (nil, nil) (the SDK answers ACCOUNT_NOT_FOUND) instead of a +// fabricated active account, and nothing is stored. Provisioning happens +// only through SyncAccounts. +func TestResolveAccountUnknownDoesNotFabricate(t *testing.T) { + b := newTestBackend() + ctx := context.Background() + + refs := map[string]adcp.AccountReference{ + "natural key": {Brand: &adcp.BrandReference{Domain: "acme-corp.com"}, Operator: "acme-corp.com"}, + "account_id": {AccountID: "acc_nope"}, + } + for name, ref := range refs { + acct, err := b.resolveAccount(ctx, ref) + if err != nil { + t.Fatalf("%s: resolveAccount: %v", name, err) + } + if acct != nil { + t.Fatalf("%s: unknown account must resolve to nil, got %v", name, acct) + } + } + if len(b.accounts) != 0 { + t.Fatalf("resolution must not create accounts, got %d", len(b.accounts)) + } + + // A provisioned account (the SyncAccounts storage shape) resolves by + // both its natural key and its returned account_id. + b.accounts["acct-acme-corp.com-acme-corp.com"] = &adcp.AccountResult{ + AccountID: "acct-acme-corp.com-acme-corp.com", Status: "active", + } + for name, ref := range refs { + if name == "account_id" { + ref = adcp.AccountReference{AccountID: "acct-acme-corp.com-acme-corp.com"} + } + acct, err := b.resolveAccount(ctx, ref) + if err != nil || acct == nil { + t.Fatalf("%s: provisioned account must resolve (acct=%v, err=%v)", name, acct, err) + } + } +} diff --git a/skills/build-seller-agent/SKILL.md b/skills/build-seller-agent/SKILL.md index 59770790..25306741 100644 --- a/skills/build-seller-agent/SKILL.md +++ b/skills/build-seller-agent/SKILL.md @@ -124,7 +124,8 @@ func main() { defer b.mu.RUnlock() domain := "" if ref.Brand != nil { domain = ref.Brand.Domain } - id := fmt.Sprintf("acct-%s-%s", domain, ref.Operator) + id := ref.AccountID // account_id references name the id sync_accounts returned + if id == "" { id = fmt.Sprintf("acct-%s-%s", domain, ref.Operator) } if acct, ok := b.accounts[id]; ok { return acct, nil } return nil, nil }, @@ -134,9 +135,11 @@ func main() { defer b.mu.Unlock() results := make([]adcp.AccountResult, 0, len(req.Accounts)) for _, acct := range req.Accounts { - domain := "unknown" - if acct.Brand != nil { domain = acct.Brand.Domain } - id := fmt.Sprintf("acct-%s-%s", domain, acct.Operator) + // Skip entries without a natural key (e.g. settings-update + // entries keyed by an `account` ref) — never fabricate + // an "acct-unknown-" account from empty fields. + if acct.Brand == nil || acct.Operator == "" { continue } + id := fmt.Sprintf("acct-%s-%s", acct.Brand.Domain, acct.Operator) result := adcp.AccountResult{AccountID: id, Brand: acct.Brand, Operator: acct.Operator, Action: "created", Status: "active"} if existing, ok := b.accounts[id]; ok { result.Action = "updated"; result.Status = existing.Status } b.accounts[id] = &result