Nice improvement -- the existing validation code was messy and complex and it's good to encapsulate it.

I have some suggestions for improving this further. Here's the API this PR introduces:

Workflow Activity

validator := activity.NewRequestAttributesValidator(...)
err := validator.ValidateAndAdjustTimeouts(runTimeout)
activityOptions := validator.GetActivityOptions()
// use activityOptions to mutate ScheduleActivityTaskCommandAttributes

Standalone Activity

validator := NewRequestAttributesValidator(...)
err = validator.ValidateAndAdjustTimeouts(durationpb.New(0))
modifiedAttributes, err := validator.ValidateStandaloneActivity()
// use modifiedAttributes to mutate StartActivityExecutionRequest

Two issues with this that I'd like to improve are

1. Someone could forget to call ValidateAndAdjustTimeouts before calling GetActivityOptions or ValidateStandaloneActivity
2. We're mutating the timeouts on the passed-in req.Options in the Standalone case; I am not sure this mutation is correct/desirable given that the request is re-used by retries.

My first idea is how about we change the API to something like

type ValidatedActivityRequest struct {
    Options                   *activitypb.ActivityOptions
    RequestID                 string
    SearchAttributesUnaliased *commonpb.SearchAttributes
}

func ValidateActivityRequest(
    activityID string,
    activityType string,
    // ...
    runTimeout *durationpb.Duration,
    standaloneAttrs *StandaloneActivityAttributes,
) (*ValidatedActivityRequest, error)

Then ValidateActivityRequest will take care of cloning input structs when necessary and adjusting the timeouts, and both code paths can use the returned struct of validated data as needed (e.g. update their own request / task attribute structs).

@dandavison - good questions, and the proto mutation is a thorn that needs improvement. We mutably sanitize the incoming request as that's what the current codebase does.

On your concerns:

1. Someone could forget to call ValidateAndAdjustTimeouts before calling GetActivityOptions or ValidateStandaloneActivity

I'm ok with that, separating out the mutated fields to a new return struct and letting the caller decide what to do. I'd rename ValidatedActivityRequest perhaps NormalizedActivityRequest as to convey the intent of mutation.

2. We're mutating the timeouts on the passed-in req.Options in the Standalone case; I am not sure this mutation is correct/desirable given that the request is re-used by retries.

Yea def a concern with the retries. If you look at StartActivityExecution in frontend.go, the request is essentially forwarded directly over to the history handler.go rpc. If we truly want to not affect retries, we should reallly clone the request with mutated fields before forwarding. The input can potentially be big. Based on your experience, do you think cloning the request will cause performance issues?

UPDATE: per discussion, we'll clone the whole request if anything needs to be mutated. The safe thing to do for predictable retries.