Please address the comments from this code review:

## Overall Comments
- The new `selectors`/`convertRulesToSelectors` path largely duplicates the old `convertRulesToLabelSelectors` logic; consider consolidating or removing the older helper to avoid divergence and reduce maintenance overhead.
- In `convertRulesToSelectors`, you eagerly allocate `namespacesByClusterID` and `namespacesByClusterName` even when there are no included namespaces; you could lazily initialize these maps in `addToNamespaceMap` to keep the zero-value of `selectors` lighter and simplify checks like `matchNamespaceByClusterKey`.

## Individual Comments

### Comment 1
<location path="pkg/sac/effectiveaccessscope/effective_access_scope.go" line_range="275-284" />
<code_context>
+	// Set the cluster state to the pre-existing state
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Cluster state aggregation keeps max state but always overwrites attributes with the latest cluster

With duplicate cluster names, `clusterState` is computed as the max of the prior state and this cluster’s state, but attributes always come from the most recently seen cluster. This can produce a mismatch where the state reflects one cluster instance while the attributes come from another. If attributes are meant to describe the cluster that determined the final (max) state, consider only updating attributes when this cluster changes `clusterState`, or explicitly define that “last cluster wins” for both state and attributes and avoid reusing the prior state.

Suggested implementation:

```golang
	clusterName := cluster.GetName()
	clusterID := cluster.GetId()
	clusterState := Excluded
	previousClusterState := Excluded

	// There is no need to check if root is Included as we start with Excluded root.
	// If root is eventually Included then we can include the cluster and short-circuit:
	// no need to match if parent is included.

	// Capture the pre-existing state, if any, so we can decide later whether to
	// update attributes based on whether this cluster changes the aggregated state.
	if clusterSubTree := root.Clusters[clusterName]; clusterSubTree != nil {
		previousClusterState = clusterSubTree.State
		clusterState = clusterSubTree.State
	}

	// Augment cluster labels with cluster's name.
	augmentedClusterLabels := augmentLabels(cluster.GetLabels(), clusterNameLabel, clusterName)

```

You’ll need to adjust the code later in this function where the aggregated `clusterState` and attributes are written back to `root.Clusters[clusterName]` (or the corresponding subtree struct), along these lines:

1. After you compute the new aggregated state for this cluster (e.g., something like `clusterState = max(clusterState, thisClusterState)`), compare it to `previousClusterState`.
2. Only overwrite attributes for this cluster when `clusterState` is strictly greater than `previousClusterState`. For example (pseudo-code, adjust to the actual types/fields):

```go
// After computing final clusterState for this cluster:
subTree, ok := root.Clusters[clusterName]
if !ok {
    subTree = &ScopeTreeNode{}
    root.Clusters[clusterName] = subTree
}

subTree.State = clusterState

// Only update attributes if this cluster actually raised the aggregate state.
if clusterState > previousClusterState {
    subTree.Attributes = computedAttributesForThisCluster
    // or whatever fields/labels you currently set from this cluster
}
```

3. If your state type is not an ordered enum, replace the `>` comparison with your existing “max”/ordering logic, but still keep the semantics: update attributes only when this cluster is the one determining the final (max) state.

This keeps `clusterState` as the max over all clusters with the same name while ensuring attributes correspond to the cluster instance that actually determined that max state.
</issue_to_address>

### Comment 2
<location path="pkg/sac/effectiveaccessscope/conversion.go" line_range="132-140" />
<code_context>
+	includedNamespaces := scopeRules.GetIncludedNamespaces()
+	output.namespacesByClusterID = make(map[string]map[string]bool)
+	output.namespacesByClusterName = make(map[string]map[string]bool)
+	for _, namespace := range includedNamespaces {
+		clusterID := namespace.GetClusterId()
+		clusterName := namespace.GetClusterName()
+		namespaceName := namespace.GetNamespaceName()
+		if clusterID != "" {
+			addToNamespaceMap(output.namespacesByClusterID, clusterID, namespaceName)
+			continue
+		}
+		addToNamespaceMap(output.namespacesByClusterName, clusterName, namespaceName)
+	}
+
</code_context>
<issue_to_address>
**issue:** Included namespaces with neither cluster ID nor name end up under an empty cluster key

With an empty `clusterId`, the code falls back to `clusterName`, which may also be empty, creating an entry under the `""` key in `namespacesByClusterName`. If namespaces without any cluster identifier are not meaningful, consider skipping them or treating them as invalid instead of aggregating them under an empty string key.
</issue_to_address>

### Comment 3
<location path="pkg/sac/effectiveaccessscope/effective_access_scope.go" line_range="262" />
<code_context>
-// name exist.
-func (root *ScopeTree) populateStateForCluster(cluster Cluster, clusterSelectors []labels.Selector, detail v1.ComputeEffectiveAccessScopeRequest_Detail) {
+// The highest selection level across observed clusters with the same name should be kept.
+func (root *ScopeTree) populateStateForCluster(
+	cluster Cluster,
+	ruleSelectors *selectors,
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving cluster and namespace matching logic into methods on the new `selectors` type and using a shared `maxScopeState` helper so that `populateStateForCluster`/`populateStateForNamespace` focus only on updating the tree.

You can keep all the new functionality while reducing duplication and branching by pushing the matching logic into `selectors` and centralizing state merging.

### 1. Encapsulate cluster matching in `selectors`

Move the ID/name/label logic out of `populateStateForCluster`:

```go
func (s *selectors) matchCluster(c Cluster) scopeState {
	clusterName := c.GetName()
	clusterID := c.GetId()

	// Augment cluster labels with cluster's name.
	augmented := augmentLabels(c.GetLabels(), clusterNameLabel, clusterName)

	state := Excluded
	if s.clustersByID[clusterID] {
		state = Included
	}
	if s.clustersByName[clusterName] {
		state = Included
	}
	state = maxScopeState(state, matchLabels(s.clustersByLabel, augmented))
	return state
}

func maxScopeState(states ...scopeState) scopeState {
	max := Excluded
	for _, s := range states {
		if s > max {
			max = s
		}
	}
	return max
}
```

Then `populateStateForCluster` becomes mostly tree-update logic:

```go
func (root *ScopeTree) populateStateForCluster(
	cluster Cluster,
	ruleSelectors *selectors,
	detail v1.ComputeEffectiveAccessScopeRequest_Detail,
) {
	clusterName := cluster.GetName()

	// Start from previous state if present
	state := Excluded
	if subTree := root.Clusters[clusterName]; subTree != nil {
		state = subTree.State
	}

	// Merge with selector result
	state = maxScopeState(state, ruleSelectors.matchCluster(cluster))

	root.Clusters[clusterName] = newClusterScopeSubTree(state, nodeAttributesForCluster(cluster, detail))
	root.clusterIDToName[cluster.GetId()] = clusterName
}
```

### 2. Encapsulate namespace matching in `selectors`

Similarly, hide the namespace ID/name/label composition in `selectors`. You can move `matchNamespaceByClusterKey` into the same type:

```go
func (s *selectors) matchNamespace(ns Namespace) scopeState {
	clusterID := ns.GetClusterId()
	clusterName := ns.GetClusterName()
	nsName := ns.GetName()
	nsFQSN := getNamespaceFQSN(clusterName, nsName)

	labels := augmentLabels(ns.GetLabels(), namespaceNameLabel, nsFQSN)

	state := Excluded
	if s.matchNamespaceByClusterKey(s.namespacesByClusterID, clusterID, nsName) {
		state = Included
	}
	if s.matchNamespaceByClusterKey(s.namespacesByClusterName, clusterName, nsName) {
		state = Included
	}
	state = maxScopeState(state, matchLabels(s.namespacesByLabel, labels))
	return state
}

func (s *selectors) matchNamespaceByClusterKey(
	targetMap map[string]map[string]bool,
	clusterKey, namespaceKey string,
) bool {
	if targetMap == nil {
		return false
	}
	m, ok := targetMap[clusterKey]
	if !ok {
		return false
	}
	return m[namespaceKey]
}
```

Then `populateStateForNamespace` reduces to:

```go
func (cluster *clustersScopeSubTree) populateStateForNamespace(
	namespace Namespace,
	ruleSelectors *selectors,
	detail v1.ComputeEffectiveAccessScopeRequest_Detail,
) {
	nsName := namespace.GetName()

	// Parent Included still short-circuits
	if cluster.State == Included {
		cluster.Namespaces[nsName] = newNamespacesScopeSubTree(Included, nodeAttributesForNamespace(namespace, detail))
		return
	}

	nsState := ruleSelectors.matchNamespace(namespace)
	cluster.Namespaces[nsName] = newNamespacesScopeSubTree(nsState, nodeAttributesForNamespace(namespace, detail))
}
```

This keeps all new capabilities (ID-based and explicit namespace rules) but:

- removes duplicated precedence logic from the callers,
- keeps `selectors` as the single place that knows about its internal maps/slices,
- makes `populateStateForCluster` and `populateStateForNamespace` primarily about tree updates rather than rule evaluation.
</issue_to_address>

Please address the comments from this code review:

## Overall Comments
- In `ScopeTree.populateStateForCluster`, when a cluster subtree already exists you return early after possibly updating its state but never update `clusterIDToName`, so additional clusters with the same name but different IDs won't be reachable via `GetClusterByID`; consider always adding `clusterIDToName[clusterID] = clusterName` even when the subtree already exists.
- In `TestSelectorsMatchNamespace`, the test case description "namespace NOT matched by label is included" contradicts the expected `Excluded` state; updating the description to match the actual expectation would make the test intent clearer.