fix: raise ValueError for unknown label selector operators

[raballew]

Summary

• _label_satisfies_expression now raises ValueError for unrecognized operators instead of silently returning False
• selector_contains falls back to _label_satisfies_expression when a required matchExpression has no matching expression in the selector but matchLabels may satisfy it
• Regression tests for empty string, invalid operators, and all valid operator paths

Closes #718

Test plan

• _label_satisfies_expression("", ...) raises ValueError
• _label_satisfies_expression("bogus", ...) raises ValueError with operator in message
• All valid operators (in, notin, exists, !exists, !=) still work correctly
• selector_contains label-to-expression fallback works for in/notin
• Existing selector tests pass unchanged

🤖 Generated with Claude Code

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Selector matching now evaluates matchExpressions against matchLabels when exact expression matches are not found. A new _label_satisfies_expression helper applies operator-specific label evaluation rules (in, notin, exists, !exists, !=), raising ValueError for unknown operators. selector_contains uses this fallback to determine expression satisfaction. LeaseList.filter_by_selector now skips leases whose selectors raise ValueError and logs a warning.

Changes

Label Selector Expression Evaluation

Layer / File(s)	Summary
Label satisfaction expression helper python/packages/jumpstarter/jumpstarter/client/selectors.py, python/packages/jumpstarter/jumpstarter/client/selectors_test.py	Added _label_satisfies_expression to evaluate whether a selector's matchLabels satisfy a label-selector expression using operator-specific rules (in, notin, exists, !exists, !=), raising ValueError for unknown/empty operators. Tests validate supported operators and that invalid operators raise ValueError with descriptive messages.
Selector matching integration python/packages/jumpstarter/jumpstarter/client/selectors.py, python/packages/jumpstarter/jumpstarter/client/selectors_test.py	Updated selector_contains to treat required matchExpressions as satisfied by exact match in selector matchExpressions or—if not found—by evaluating against selector matchLabels using _label_satisfies_expression. Tests expanded to cover in(...), notin(...), !=, !exists, exists, and whitespace/operator-format tolerance.
gRPC LeaseList selector-filter resilience python/packages/jumpstarter/jumpstarter/client/grpc.py, python/packages/jumpstarter/jumpstarter/client/grpc_test.py	Added a module logger and changed LeaseList.filter_by_selector to iterate leases and skip those where selector_contains raises ValueError, logging a warning. Tests added to verify errored leases are omitted from the filtered result.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• jumpstarter-dev/jumpstarter#174: Related updates around selector/matchExpression handling and client-side filtering; #174 extracts matchLabels while this PR evaluates matchExpressions against those labels.

Suggested reviewers

• evakhoni
• maboras-rh
• mangelajo

Poem

🐰 In a patch of code I hopped with glee,
exact matches first, then labels I see;
"in", "notin", "exists" — I test each rule,
unknown ops cry out, they break the pool,
I skip the bad leases and nibble on tea. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.82% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing ValueError behavior for unknown label selector operators, which is the primary objective of the PR.
Description check	✅ Passed	The description is directly related to the changeset, documenting the specific fixes applied including ValueError behavior, fallback logic, and test coverage.
Linked Issues check	✅ Passed	The PR fully addresses issue #718 by modifying _label_satisfies_expression to raise ValueError for unknown operators, implementing all required fixes and test coverage as specified.
Out of Scope Changes check	✅ Passed	All changes are within scope: _label_satisfies_expression now raises ValueError for unknown operators, selector_contains implements fallback logic, and comprehensive tests verify the new behavior.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

python/packages/jumpstarter/jumpstarter/client/selectors.py (1)

89-93: 💤 Low value

Consider updating the docstring to reflect fallback behavior.

The docstring states matchExpressions must be "present" in selector, but the new implementation allows matchLabels to satisfy matchExpressions via _label_satisfies_expression. The inline comment at lines 106-107 accurately describes "satisfied by selector's matchExpressions or matchLabels," but the function docstring could be updated to match.

📝 Suggested docstring update

 def selector_contains(selector: str, requirements: str) -> bool:
-    """Check if selector contains all criteria from requirements.
+    """Check if selector satisfies all criteria from requirements.
 
-    Returns True if all matchLabels and matchExpressions in `requirements`
-    are present in `selector`.
+    Returns True if all matchLabels in `requirements` are present in `selector`
+    and all matchExpressions in `requirements` are satisfied by `selector`
+    (either by exact match in matchExpressions or by evaluation against matchLabels).
     """

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/packages/jumpstarter/jumpstarter/client/selectors.py` around lines 89
- 93, The selector_contains docstring is out-of-date about matchExpressions:
update the docstring for selector_contains to state that requirements'
matchExpressions may be satisfied either by selector's matchExpressions or by
selector's matchLabels (via the helper _label_satisfies_expression), and clarify
the fallback behavior so the function docstring matches the inline comment and
current implementation.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/selectors.py`:
- Around line 89-93: The selector_contains docstring is out-of-date about
matchExpressions: update the docstring for selector_contains to state that
requirements' matchExpressions may be satisfied either by selector's
matchExpressions or by selector's matchLabels (via the helper
_label_satisfies_expression), and clarify the fallback behavior so the function
docstring matches the inline comment and current implementation.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f597acb7-6ece-444b-ad51-d76db918a07b

📥 Commits

Reviewing files that changed from the base of the PR and between d706fa7 and c0b59d2.

📒 Files selected for processing (2)

• python/packages/jumpstarter/jumpstarter/client/selectors.py
• python/packages/jumpstarter/jumpstarter/client/selectors_test.py

[mangelajo]

see the comments

[coderabbitai]

🧹 Nitpick comments (1)

python/packages/jumpstarter/jumpstarter/client/grpc.py (1)

381-383: ⚡ Quick win

Include the caught error text in the warning.

Line 382 logs that evaluation failed but drops the actual ValueError, which makes malformed selector diagnosis harder.

♻️ Suggested patch

-            except ValueError:
-                logger.warning("skipping lease %s: unable to evaluate selector %r", lease.name, lease.selector)
+            except ValueError as exc:
+                logger.warning(
+                    "skipping lease %s: unable to evaluate selector %r (%s)",
+                    lease.name,
+                    lease.selector,
+                    exc,
+                )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/packages/jumpstarter/jumpstarter/client/grpc.py` around lines 381 -
383, The warning for a failed selector evaluation in the Lease filtering code
drops the ValueError details; update the except ValueError handler in the
function that builds the LeaseList (where lease, lease.selector and filtered are
used) to capture the exception (e.g., except ValueError as e) and include the
exception text in the logger.warning call so the message includes both the lease
name, selector and the caught error information for easier diagnosis.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/grpc.py`:
- Around line 381-383: The warning for a failed selector evaluation in the Lease
filtering code drops the ValueError details; update the except ValueError
handler in the function that builds the LeaseList (where lease, lease.selector
and filtered are used) to capture the exception (e.g., except ValueError as e)
and include the exception text in the logger.warning call so the message
includes both the lease name, selector and the caught error information for
easier diagnosis.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e0048d6c-538c-4151-af6b-ed75ec87a5f1

📥 Commits

Reviewing files that changed from the base of the PR and between dc7a8d4 and 5f3aa81.

📒 Files selected for processing (2)

• python/packages/jumpstarter/jumpstarter/client/grpc.py
• python/packages/jumpstarter/jumpstarter/client/grpc_test.py

[raballew]

You create two exporters:

• Exporter A: labels board=sa, selector requires !nonexistent
• Exporter B: labels board=sa, selector requires !production

You run jmp get leases --selector board=sa,!production

@maboras-rh @mangelajo what do you expect to see?

[mangelajo]

You create two exporters:

• Exporter A: labels board=sa, selector requires !nonexistent
• Exporter B: labels board=sa, selector requires !production

You run jmp get leases --selector board=sa,!production

@maboras-rh @mangelajo what do you expect to see?

I think we should just do whatever kubernetes does in this case, which... I don't know what it is :D

[maboras-rh]

You create two exporters:

• Exporter A: labels board=sa, selector requires !nonexistent
• Exporter B: labels board=sa, selector requires !production

You run jmp get leases --selector board=sa,!production

@maboras-rh @mangelajo what do you expect to see?

this is a valid design question. The fallback treats the selector's matchLabels as the complete label set. But a lease selector is a constraint, not a full description of an exporter's labels. Exporter A's selector
(board=sa,!nonexistent) only tells us:

• The exporter has board=sa
• The exporter doesn't have a nonexistent label

It says nothing about whether the exporter has a production label. the actual exporter behind that lease could have production=true - the selector just didn't check for it. So the fallback's conclusion that "production doesn't exist" is an assumption that may not hold.
my expectation is that only Exporter B should be shown. We can only guarantee that B's exporter doesn't have production (its selector explicitly requires !production). For A, we simply don't know(and showing it could be a false positive)

[raballew]

and even this might not be true because the exporter object could have changed in the meantime - so the lease holds a snapshot of an old exporter? In this case the lease would need to carry its own context and we have two options:

1. snapshot the exporters labels at lease creation time
2. remove the fallback

[raballew]

k8s has no concept of "one selector contains another". It only evaluates selectors against concrete label sets. selector_contains is a custom abstraction - it treats one selector's matchLabels as a proxy for resource labels and evaluates the other selector's expressions against them which has no k8s counterpart.

[raballew]

The latest commit fixes Kubernetes compatibility for notin and != when the key is absent from the label set.

In Kubernetes, NotIn and NotEquals return true when the key is missing — the resource trivially satisfies "not in" or "not equal" because it has no value for that key. The previous implementation returned false for all operators (except !exists) when the key was absent.

Concrete examples that were wrong before:

• selector_contains("board=rpi", "env notin (prod)") returned False, now returns True
• selector_contains("board=rpi", "env!=prod") returned False, now returns True

The fix moves notin and != before the key-absence guard in _label_satisfies_expression, matching the behavior of Go's k8s.io/apimachinery/pkg/labels package.

Tests now cover all 7 Kubernetes label selector operators (=, ==, !=, in, notin, exists, !exists) with both key-present and key-absent cases.