PR 19787, round 2: `convert()` fixed where the bug was

By codyrancher, milestone v2.17.0. Round 2 reviewed 2026-11-10, two passes (Cody's process and ours) merged.

Preview: https://rancherpr19787r.13.48.147.135.sslip.io/dashboard/ rebuilt from e243cca96, backend https://rancher.13.48.147.135.sslip.io (Rancher v2.16 head). The round-1 build is gone; the fork build pr19787conv is still up.

Verdict: approve. Both passes agree. Cody took your suggestion: convert() now builds its own list and keeps each label as its own rule, and the clones are gone. Your round-1 comment is addressed, checked live. No blockers. One request about the PR description and one optional nit. Nothing is posted on GitHub.

Your approval was dismissed You submitted it on 11-06 with your own text. Cody's push on 11-09 (a rebase plus the new commit) dismissed it, and Cody re-requested review this morning, so the PR needs a fresh approval.

What changed since round 1

The original commit was rebased onto the new master (2b45c9dda → cafb8f889). git range-diff shows it unchanged.
e243cca96 "Stop convert() changing the selector it is given" rewrites convert() and removes the three clones. Against master the PR is now selector.js (+9/−8) plus tests; neither network policy .vue file changes any more.
Cody replied on your thread: two rows and 0 pods on the mixed fixture, the PodDisruptionBudget form sends each rule once, and selectors already saved with the duplicates show one row.
CI on e243cca96 is green, unit tests, lint, type-check and all e2e jobs included. Branch Milestone is red on purpose.

Your round-1 comment: addressed

I ran the round-1 walk again against the new build, with the Harvester extension blocked and every write aborted, so each result is the request the form would send. Every case gives the same result as our fork build did on 11-06.

Round 1 PR build: one row aaa in list bbb, aaa, banner Matches 3 of 5 existing pods, three pods listed — Round 1 build: the label merged into the rule, 3 of 5 pods.

e243cca96: two rows aaa in list bbb and aaa in list aaa, banner Matches 0 of 5 pods, no pods listed — `e243cca96`: two rows, 0 of 5 pods, as kubectl says.

Case	Round 1 build	`e243cca96`
New PodDisruptionBudget, two rules	each rule sent twice	sent once, as labels
Monitoring install, `aaa` plus `bbb NotIn x`	`aaa` sent twice	sent once
Mixed policy `aaa=aaa` plus `aaa In [bbb]`	one row, 3 of 5 pods	two rows, 0 of 5
Retype that rule and save	saves the wider `aaa in (bbb,aaa)`	saves both rules, still 0 pods
Ingress rule targets, same selector	3 of 5 pods, 1 of 174 namespaces	0 of 5, 0 of 176
Policy already saved with duplicates	one row	one row, sent unchanged
Save with no changes: ClusterGroup, PodDisruptionBudget, autoscaler	rewritten	unchanged
Plain selectors, other operators, Clone, workload and ReplicaSet pods	fine	same

The new `convert()` next to ours

It is the change from your fork branch (zz-19787-convert-rewrite) with two differences. Neither matters in practice.

Duplicate check. Cody compares with isEqual against the label as a one-value list. Ours compared single values. Same result for every real label. Cody's version also skips an array-valued label, where ours didn't.
No isArray guard. A string matchExpressions is split into characters, and an object or number throws. Only a hand-edited chart values file can hold one, the API rejects it on every resource, and master already throws there once a label exists. Not raised.

The round-1 harness, rerun with three versions over 2,457 generated selectors: Cody's output equals ours in every case. Neither writes to its input. Both agree with Kubernetes on 22,113 pod checks, where master is wrong 972 times. The copy is shallow, but MatchExpressions clones every rule before editing it and no caller writes to the result.

The two passes

	Cody's process (ai-skills, second-round skill)	Our process (fresh agent)
Verdict	Approve: 1 comment addressed, 0 new	Approve: 1 suggestion, 1 optional nit
Round-1 comment	Addressed, round-1 walk rerun live	Addressed, each claim in Cody's reply checked live
Stale PR description	Noticed, not filed (that skill files code problems only)	Filed as a suggestion
Docblock says what, not why	Not filed (no nits)	Optional nit
Checked by both	Mixed fixture, PodDisruptionBudget create, Monitoring install, already-doubled policies, ClusterGroup Save, autoscaler, no regression in any other `convert()` caller
Only this pass	Side-by-side harness against our fork, GitHub search for extension callers, workload and ReplicaSet pod lists	PR's tests run against master's `convert()` (9 fail, so they guard the fix), per-caller code read, two pre-existing bugs below
Cost	In this session, about 25 minutes	About 18 minutes, 92 tool calls

Neither pass changed anything on mf. The shared admin's language is set to Traditional Chinese by someone else. Both passes switched it to English in their own browser only, and its saved value is unchanged.

Findings that survived

REQUESTThe PR description still describes round 1

The technical notes still say convert() writes into the array it is given, "hence the deep copy", and that fixing convert() is "left for a follow-up". This PR is now that follow-up. The regression areas list only network policies, but convert() also backs the PodDisruptionBudget, Monitoring storage and autoscaler metric selectors, Fleet cluster groups and GitRepo cluster targets, and the matching pods lists. QA plans from that section. It should also say that a label and an In rule on the same key now show as two rows. No code change needed.

OPTIONALThe `convert()` docblock says what, not why

selector.js:125: the reason a label must not join an In rule on the same key is that they are ANDed, and merging widens the selector. That is what someone could "simplify" back later. The new test at selector.test.ts:267 already guards it, so I'd leave this out unless you want it.

Already on master, not filed

Autoscaler metric selector edits are dropped and log TypeError: e is not iterable. This is section 3 of open issue 19668. That issue says fixing it puts the term in both forms. That was the old convert(), so with this PR it can be fixed on its own.
The autoscaler and Monitoring storage selector getters show no rows when the stored selector has labels only. Both return an empty list unless matchExpressions exists (metric-identifier.vue:28, prometheus/index.vue:73). From reading the code, not run live.
The Fleet ClusterGroup edit form rewrites the selector as soon as it opens. With this PR the rewrite means the same thing. On master it widened mixed selectors.

Dropped, and why

The missing isArray guard: not reachable in normal use (above).
Extensions calling convert(): they bundle their own shell. The only public caller found, Harvester's node selector, uses the return value and gets the same fix when it updates.
A mixed selector saved as two In rules after an edit: same meaning, different shape. simplify() can't fold two rules on one key back into a label.
One new test, "an identical In rule", also passes on master. It documents the duplicate skip, while the other new tests guard the fix.

What I'd post (Approve)

Body

Thanks for changing convert() itself. I tested it again on a real Rancher. Your mixed fixture shows two rows and matches no pods. The PodDisruptionBudget form and the Monitoring storage selector send each rule once. Selectors already saved with the duplicates show one row. Save with no changes sends the selector unchanged on network policies, PodDisruptionBudgets, autoscalers and Fleet cluster groups. LGTM.

One thing before merging: could you update the description? It still says fixing convert() is left for a follow-up, and the regression areas only cover network policies. Since convert() is shared, it would help QA to list the other places it reaches: the PodDisruptionBudget, Monitoring storage and autoscaler metric selectors, Fleet cluster groups and cluster targets, and the matching pods lists. It's also worth saying that a label and an In rule on the same key now show as two rows.

Optional inline comment on `shell/utils/selector.js` line 125

Optional: a few words on why a label stays its own rule would help here. A label and an In rule are ANDed, so merging them widens the selector, and that is the part someone might "simplify" back later.

Your round-1 thread can be resolved once you submit.

Left on mf

Previews dashboard-pr19787 (now e243cca96) and dashboard-pr19787conv. Pages pr19787-review on mf and this one on created-dashboard-2. Namespaces zz-pr19787, zz-pr19787-peer and zz-pr19787-conv, and ClusterGroup fleet-default/zz-pr19787-cg-mixed. Round 2 created nothing.

PR 19787, round 2: convert() fixed where the bug was